Skip to content

Commit 8682131

Browse files
authored
RecordQueryExplodePlan plan adheres to skip and limit requirements (#3230)
The `RecordQueryExplodePlan` plan is, so far, only used internally to represent a transposition of constant arrays such as e.g. `in` values as a leg of an in-join plan, and as long as the upper operator honors the skip and limit requirements it was probably ok to have this operator ignore these parameters internally. In fact, it is somewhat common for the upper operator to remove these requirements before proceeding to execute the inner operator. I came across this issue when implementing table-functions, one example of these functions is to directly select from `values` that translates to a projection from this operator at runtime. In other words, it suddenly became a top-level operator, and therefore, it must respect skip and limit to align with the execution behavior of other operators. This PR explicitly sets the `limit` and `skip` on the resulting `RecordCursor` of the `RecordQueryExplodePlan`, it also adds a number of unit tests. It resolves #3229.
1 parent 7f7d633 commit 8682131

File tree

2 files changed

+140
-1
lines changed

2 files changed

+140
-1
lines changed

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,8 @@ public <M extends Message> RecordCursor<QueryResult> executePlan(@Nonnull final
8888
@Nonnull final ExecuteProperties executeProperties) {
8989
final var result = collectionValue.eval(store, context);
9090
return RecordCursor.fromList(result == null ? ImmutableList.of() : (List<?>)result, continuation)
91-
.map(QueryResult::ofComputed);
91+
.map(QueryResult::ofComputed)
92+
.skipThenLimit(executeProperties.getSkip(), executeProperties.getReturnedRowLimit());
9293
}
9394

9495
@Nonnull
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
/*
2+
* ExplodePlanTest.java
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2015-2025 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
package com.apple.foundationdb.record.query.plan.plans;
22+
23+
import com.apple.foundationdb.record.EvaluationContext;
24+
import com.apple.foundationdb.record.ExecuteProperties;
25+
import com.apple.foundationdb.record.RecordCursor;
26+
import com.apple.foundationdb.record.query.plan.cascades.values.LiteralValue;
27+
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
28+
import com.google.common.collect.ImmutableList;
29+
import org.junit.jupiter.api.Assertions;
30+
import org.junit.jupiter.api.extension.ExtensionContext;
31+
import org.junit.jupiter.params.ParameterizedTest;
32+
import org.junit.jupiter.params.provider.Arguments;
33+
import org.junit.jupiter.params.provider.ArgumentsProvider;
34+
import org.junit.jupiter.params.provider.ArgumentsSource;
35+
36+
import javax.annotation.Nonnull;
37+
import java.util.List;
38+
import java.util.Objects;
39+
import java.util.Optional;
40+
import java.util.stream.Stream;
41+
42+
public class ExplodePlanTest {
43+
44+
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
45+
private static final class ExplodeCursorBuilder {
46+
47+
@Nonnull
48+
private final RecordQueryPlan explodePlan;
49+
50+
@Nonnull
51+
private Optional<Integer> skip;
52+
53+
@Nonnull
54+
private Optional<Integer> limit;
55+
56+
private ExplodeCursorBuilder() {
57+
explodePlan = generateExplodePlan();
58+
skip = Optional.empty();
59+
limit = Optional.empty();
60+
}
61+
62+
@Nonnull
63+
ExplodeCursorBuilder withSkip(int skip) {
64+
this.skip = Optional.of(skip);
65+
return this;
66+
}
67+
68+
@Nonnull
69+
ExplodeCursorBuilder withLimit(int limit) {
70+
this.limit = Optional.of(limit);
71+
return this;
72+
}
73+
74+
@Override
75+
public String toString() {
76+
return "explode [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] "
77+
+ limit.map(l -> "limit " + l + " ").orElse("")
78+
+ skip.map(s -> "skip " + s).orElse("");
79+
}
80+
81+
@Nonnull
82+
@SuppressWarnings("DataFlowIssue") // explode transposes the underlying constant array Value, it does not strictly require a record store instance.
83+
RecordCursor<QueryResult> build() {
84+
final var executionPropertiesBuilder = ExecuteProperties.newBuilder();
85+
skip.ifPresent(executionPropertiesBuilder::setSkip);
86+
limit.ifPresent(executionPropertiesBuilder::setReturnedRowLimit);
87+
final var executionProperties = executionPropertiesBuilder.build();
88+
return explodePlan.executePlan(null, EvaluationContext.EMPTY, null, executionProperties);
89+
}
90+
91+
@Nonnull
92+
private static RecordQueryPlan generateExplodePlan() {
93+
final Value collectionValue = LiteralValue.ofList(List.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10));
94+
return new RecordQueryExplodePlan(collectionValue);
95+
}
96+
97+
@Nonnull
98+
public static ExplodeCursorBuilder instance() {
99+
return new ExplodeCursorBuilder();
100+
}
101+
}
102+
103+
private static void verifyCursor(@Nonnull final RecordCursor<QueryResult> actualCursor,
104+
@Nonnull final List<Integer> expectedResults,
105+
boolean verifyLimitExceeded) {
106+
for (final var expectedValue : expectedResults) {
107+
final var result = actualCursor.getNext();
108+
Assertions.assertTrue(result.hasNext());
109+
Assertions.assertNotNull(result.get());
110+
Assertions.assertEquals(expectedValue, Objects.requireNonNull(result.get()).getDatum());
111+
}
112+
if (verifyLimitExceeded) {
113+
final var result = actualCursor.getNext();
114+
Assertions.assertFalse(result.hasNext());
115+
Assertions.assertEquals(RecordCursor.NoNextReason.RETURN_LIMIT_REACHED, result.getNoNextReason());
116+
}
117+
}
118+
119+
private static class ArgumentProvider implements ArgumentsProvider {
120+
@Override
121+
public Stream<? extends Arguments> provideArguments(final ExtensionContext context) {
122+
return Stream.of(
123+
Arguments.of(ExplodeCursorBuilder.instance().withLimit(1), ImmutableList.of(1), true),
124+
Arguments.of(ExplodeCursorBuilder.instance().withLimit(4), ImmutableList.of(1, 2, 3, 4), true),
125+
Arguments.of(ExplodeCursorBuilder.instance().withLimit(1).withSkip(3), ImmutableList.of(4), true),
126+
Arguments.of(ExplodeCursorBuilder.instance().withLimit(2).withSkip(5), ImmutableList.of(6, 7), true),
127+
Arguments.of(ExplodeCursorBuilder.instance(), ImmutableList.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), false));
128+
}
129+
}
130+
131+
@ParameterizedTest(name = "{0} should return {1}")
132+
@ArgumentsSource(ArgumentProvider.class)
133+
void explodeWithSkipAndLimitWorks(@Nonnull final ExplodeCursorBuilder actualCursorBuilder,
134+
@Nonnull final List<Integer> expectedResult,
135+
boolean shouldReachLimit) {
136+
verifyCursor(actualCursorBuilder.build(), expectedResult, shouldReachLimit);
137+
}
138+
}

0 commit comments

Comments
 (0)