Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set skip and limit for the recursive union cursor correctly. #3111

Merged
merged 3 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,18 @@ public <M extends Message> RecordCursor<QueryResult> executePlan(@Nonnull final
@Nullable final byte[] continuation,
@Nonnull final ExecuteProperties executeProperties) {
final var type = getInnerTypeDescriptor(context);
final var childExecuteProperties = executeProperties.clearSkipAndLimit();
final var recursiveStateManager = new RecursiveStateManagerImpl(
(initialContinuation, evaluationContext) -> getInitialStatePlan().executePlan(store, evaluationContext,
initialContinuation == null ? null : initialContinuation.toByteArray(), executeProperties),
initialContinuation == null ? null : initialContinuation.toByteArray(), childExecuteProperties),
(recursiveContinuation, evaluationContext) -> getRecursiveStatePlan().executePlan(store, evaluationContext,
recursiveContinuation == null ? null : recursiveContinuation.toByteArray(), executeProperties),
recursiveContinuation == null ? null : recursiveContinuation.toByteArray(), childExecuteProperties),
context,
tempTableScanAlias,
tempTableInsertAlias,
proto -> TempTable.from(proto, type), store.getContext().getTempTableFactory(), continuation);
return new RecursiveUnionCursor<>(recursiveStateManager, store.getExecutor());
return new RecursiveUnionCursor<>(recursiveStateManager, store.getExecutor())
.skipThenLimit(executeProperties.getSkip(), executeProperties.getReturnedRowLimit());
}

@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.apple.foundationdb.relational.util.SpotBugsSuppressWarnings;

import com.google.common.collect.HashMultiset;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Multiset;
import com.google.protobuf.Descriptors;
import com.google.protobuf.Message;
Expand Down Expand Up @@ -428,7 +429,8 @@ private static ImmutablePair<ResultSetMatchResult, ResultSetPrettyPrinter> match
var i = 1;
for (final var expectedRow : expectedAsList) {
if (!actual.next()) {
return ImmutablePair.of(ResultSetMatchResult.fail("result does not contain all expected rows."), null);
printCurrentAndRemaining(actual, resultSetPrettyPrinter);
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("result does not contain all expected rows! expected %d rows, got %d", expectedAsList.size(), i - 1)), null);
}
if (!isMap(expectedRow)) { // I think it should be possible to expect a result set like: [[1,2,3], [4,5,6]]. But ok for now.
printCurrentAndRemaining(actual, resultSetPrettyPrinter);
Expand All @@ -449,20 +451,24 @@ private static ImmutablePair<ResultSetMatchResult, ResultSetPrettyPrinter> match

private static ImmutablePair<ResultSetMatchResult, ResultSetPrettyPrinter> matchUnorderedResultSet(final RelationalResultSet actual, final @Nonnull Multiset<?> expectedAsMultiSet) throws SQLException {
final ResultSetPrettyPrinter resultSetPrettyPrinter = new ResultSetPrettyPrinter();
var i = 1;
// O(n^2) -- we all got M1s
normen662 marked this conversation as resolved.
Show resolved Hide resolved
final var expectedRowCount = expectedAsMultiSet.size();
var actualRowsCounter = 1;
while (actual.next()) {
boolean found = false;
if (expectedAsMultiSet.isEmpty()) {
printCurrentAndRemaining(actual, resultSetPrettyPrinter);
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("too many rows in actual result set! expected %d rows, got %d.", i - 1, i - 1 + resultSetPrettyPrinter.getRowCount())), resultSetPrettyPrinter);
// count the remaining actual rows.
while (actual.next()) {
actualRowsCounter++;
}
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("too many rows in actual result set! expected %d row(s), got %d row(s) instead.", expectedRowCount, actualRowsCounter - 1)), resultSetPrettyPrinter);
}
for (final var expectedRow : expectedAsMultiSet) {
for (final var expectedRow : expectedAsMultiSet.elementSet()) {
if (!isMap(expectedRow)) { // I think it should be possible to expect a result set like: [[1,2,3], [4,5,6]]. But ok for now.
printCurrentAndRemaining(actual, resultSetPrettyPrinter);
return ImmutablePair.of(ResultSetMatchResult.fail("unknown format of expected result set"), resultSetPrettyPrinter);
}
final var matchResult = matchRow(map(expectedRow), actual.getMetaData().getColumnCount(), valueByName(actual), valueByIndex(actual), i);
final var matchResult = matchRow(map(expectedRow), actual.getMetaData().getColumnCount(), valueByName(actual), valueByIndex(actual), actualRowsCounter);
if (matchResult.equals(ResultSetMatchResult.success())) {
found = true;
expectedAsMultiSet.remove(expectedRow);
Expand All @@ -471,12 +477,12 @@ private static ImmutablePair<ResultSetMatchResult, ResultSetPrettyPrinter> match
}
if (!found) {
printCurrentAndRemaining(actual, resultSetPrettyPrinter);
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("result row at %d does not match any expected records", i)), resultSetPrettyPrinter);
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("actual row at %d does not match any expected records", actualRowsCounter)), resultSetPrettyPrinter);
}
i++;
actualRowsCounter++;
}
if (!expectedAsMultiSet.isEmpty()) {
return ImmutablePair.of(ResultSetMatchResult.fail("result does not contain all expected rows"), null);
return ImmutablePair.of(ResultSetMatchResult.fail(String.format("result does not contain all expected rows, expected %d row(s), got %d row(s) instead.", expectedRowCount, actualRowsCounter - 1)), null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is tests, but I think we want to refrain from String.format (especially without a locale) if we can avoid it. That does seem to be a larger problem in this file, though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made all the String.format calls here use Locale.ROOT. I will propose a separate PR that extends this fix to the entire codebase.

}
return ImmutablePair.of(ResultSetMatchResult.success(), null);
}
Expand Down
8 changes: 8 additions & 0 deletions yaml-tests/src/test/resources/cte.yamsql
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,12 @@ test_block:
# TODO (CTEs and subqueries are not fully optimized)
- query: with c1(x, y) as (select col1, col2 from t1) select x from c1 where y < 3
- explain: "SCAN(<,>) | MAP (_.COL1 AS COL1, _.COL2 AS COL2) | FILTER _.COL2 LESS_THAN promote(@c24 AS LONG) | MAP (_.COL1 AS X)"
-
- query: with c1 as (select col1, col2 from t1) select col1, col2 from c1
- maxRows: 1
- result: [{COL1: 10, COL2: 1}]
- result: [{COL1: 10, COL2: 2}]
- result: [{COL1: 20, COL2: 6}]
- result: [{COL1: 20, COL2: 7}]
- result: []
...
43 changes: 43 additions & 0 deletions yaml-tests/src/test/resources/recursive-cte.yamsql
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,49 @@ test_block:
{210, 20},
{250, 50},
{250, 50}]
-
- query: with recursive c1 as (
select id, parent from t1 where parent = -1
union all
select b.id, b.parent from c1 as a, t1 as b where a.id = b.parent) select id from c1
- maxRows: 1
- result: [{ID: 1}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is an outstanding PR that does this stuff automatically for up/downlevel testing. So you may not need these tests in a week from now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I would leave it for now not to lose test coverage, and we can clean up later when we have a way to automatically test continuations in YAML.

- result: [{ID: 10}]
- result: [{ID: 20}]
- result: [{ID: 40}]
- result: [{ID: 50}]
- result: [{ID: 70}]
- result: [{ID: 100}]
- result: [{ID: 210}]
- result: [{ID: 250}]
- result: []
-
- query: with recursive allDescendants as (
with recursive ancestorsOf250 as (
select id, parent from t1 where id = 250
union all
select b.id, b.parent from ancestorsOf250 as a, t1 as b where a.parent = b.id) select id, parent from ancestorsOf250
union all
select b.id, b.parent from allDescendants as a, t1 as b where a.id = b.parent) select id, parent from allDescendants
- maxRows: 1
- result: [{250, 50}]
- result: [{50, 10}]
- result: [{10, 1}]
- result: [{1, -1}]
- result: [{10, 1}]
- result: [{20, 1}]
- result: [{40, 10}]
- result: [{50, 10}]
- result: [{70, 10}]
- result: [{250, 50}]
- result: [{40, 10}]
- result: [{50, 10}]
- result: [{70, 10}]
- result: [{100, 20}]
- result: [{210, 20}]
- result: [{250, 50}]
- result: [{250, 50}]
- result: []
# -
# does not currently work due to bug in NLJ planning, see https://github.com/FoundationDB/fdb-record-layer/issues/2997
# - query: with recursive c1 as (
Expand Down
Loading