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

Fix the bug when aggregation stops before a group is finished #3177

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

pengpeng-lu
Copy link
Contributor

@pengpeng-lu pengpeng-lu commented Feb 19, 2025

  • Problem: in aggregate queries, if a scan is stopped in the middle of a group, we'll just return the current result, which is wrong, and continuation = current row, so the next scan will also return wrong result.
  • Solution: doesn't return anything if we stop in the middle of a group, and resets the continuation to the last group, to prepare for the next scan.

This resolves #3180

.build(false);

// In the testing data, there are 2 groups, each group has 3 rows.
// recordScanLimit = 5: scans 3 rows, and the 4th scan hits SCAN_LIMIT_REACHED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why it only scans 4 times instead of 5, but the offset=1 seems consistent.

@pengpeng-lu pengpeng-lu changed the title Fix the bug when aggregation stops before a group is finished Resolves #3180: Fix the bug when aggregation stops before a group is finished Feb 19, 2025
@ScottDugas ScottDugas changed the title Resolves #3180: Fix the bug when aggregation stops before a group is finished Fix the bug when aggregation stops before a group is finished Feb 20, 2025
@ScottDugas ScottDugas added the bug fix Change that fixes a bug label Feb 20, 2025
@alecgrieser
Copy link
Collaborator

It looks like there's a conflict in the release notes. PRs no longer need to update the release notes file as they will be auto-generated by #3174, so I think the solution is just "take what is on main"

// last row in last group, is null if the current group is the first group
@Nullable
private RecordCursorResult<QueryResult> lastInLastGroup;
byte[] continuation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
byte[] continuation;
@Nullable
private byte[] continuation;

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this still hasn't marked the continuation field as private. Is there a reason it needs package visibility?

@@ -20,7 +20,7 @@ Users performing online updates are encouraged to update from [4.0.559.4](#40559
<h4> New Features </h4>

* FRL respects PLAN_CACHE_*_MAX_ENTRIES - [PR #3156](https://github.com/FoundationDB/fdb-record-layer/pull/3156)
<h4> Bug Fixes </h4>
<h4> Bug Fixes </h4>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what happened here, but we probably want to back this out and take what's on main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// last row in last group, is null if the current group is the first group
@Nullable
private RecordCursorResult<QueryResult> lastInLastGroup;
byte[] continuation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this still hasn't marked the continuation field as private. Is there a reason it needs package visibility?

} else {
if (Verify.verifyNotNull(previousResult).getNoNextReason() == NoNextReason.SOURCE_EXHAUSTED) {
if (previousValidResult == null) {
return RecordCursorResult.exhausted();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to check to insert the empty result if isCreateDefaultOnEmpty? It feels like that information is being lost here.

Perhaps it would be simpler if there were fewer moving parts here. If we wait until 4.2, we could drop support entirely for isCreateDefaultOnEmpty (see #3107) which may simplify this somewhat.

currentContinuation = lastInLastGroup.getContinuation();
}
previousValidResult = lastInLastGroup;
return RecordCursorResult.withoutNextValue(currentContinuation, Verify.verifyNotNull(previousResult).getNoNextReason());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also set previousResult? I think as written, if you call getNext a second time after getting one of these out-of-bounds values, you'll get an advanced continuation.

That is:

RecordCursor<T> aggCursor = getAggregateCursor();
RecordCursorResult<T> withValueResult = aggCursor.getNext(); // has a value
RecordCursorResult<T> withoutValueResult1 = aggCursor.getNext(); // first result with out-of-bound limit; continuation borrowed from withValueResult
RecordCursorResult<T> withoutValueResult2 = aggCursor.getNext(); // second result with out-of-bound limit; continuation may be greater than withoutValueResult1

In general, once a cursor returns a RecordCursorResult without a next value, it should continue to return the same result if getNext() is returned again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Change that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aggregation result is incorrect when a scan is stopped in the middle of a group
3 participants