Skip to content

Add partial result to AggregateCursor continuation #3254

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

Merged
merged 55 commits into from
Jun 6, 2025

Conversation

pengpeng-lu
Copy link
Contributor

@pengpeng-lu pengpeng-lu commented Mar 17, 2025

  1. Add AggregateCursor.AggregateCursorContinuation, which contains partialAggregationResult.
  2. AggregateCursor returns AggregateCursor.AggregateCursorContinuation
  3. Add tests in record-layer and relational-layer to verify

@pengpeng-lu pengpeng-lu added the enhancement New feature or request label Mar 17, 2025
(prefixLength < highBytes.length) &&
(lowBytes[prefixLength] == highBytes[prefixLength])) {
(prefixLength < highBytes.length) &&
(lowBytes[prefixLength] == highBytes[prefixLength])) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird auto formating, will remove it in the next commit.

@pengpeng-lu pengpeng-lu requested a review from alecgrieser May 5, 2025 22:45
}

@Nonnull
@Override
public CompletableFuture<RecordCursorResult<QueryResult>> onNext() {
if (previousResult != null && !previousResult.hasNext()) {
// we are done
return CompletableFuture.completedFuture(RecordCursorResult.exhausted());
return CompletableFuture.completedFuture(RecordCursorResult.withoutNextValue(new AggregateCursorContinuation(previousResult.getContinuation(), serializationMode),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this also needs to include the partialAggregationResult, though. I'm thinking of a case like:

try (RecordCursor<QueryResult> cursor = getAggregateCursor()) {
   RecordCursor<QueryResult> curr = cursor.getNext();
   while (curr.hasNext()) {
     // do something with curr
     curr = cursor.getNext();
   }

   RecordCursor<QueryResult> last = cursor.getNext();
   assert !last.hasNext(); // should succeed
   assert last.getContinution().equals(curr.getContinuation()); // I think will fail as last is missing any partial aggregation result 
}

The last value there is the second result from the cursor where .hasNext() is false.

@pengpeng-lu pengpeng-lu requested a review from alecgrieser May 27, 2025 19:52
}

@Nonnull
@Override
public CompletableFuture<RecordCursorResult<QueryResult>> onNext() {
if (previousResult != null && !previousResult.hasNext()) {
// we are done
return CompletableFuture.completedFuture(RecordCursorResult.exhausted());
return CompletableFuture.completedFuture(RecordCursorResult.withoutNextValue(new AggregateCursorContinuation(previousResult.getContinuation(), serializationMode),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the partialAggregationResult at the moment. And did you add a test that called .getNext() a second time on a partially completed cursor?

@pengpeng-lu pengpeng-lu requested a review from alecgrieser June 2, 2025 05:10
@pengpeng-lu pengpeng-lu removed the request for review from normen662 June 6, 2025 20:15
@pengpeng-lu pengpeng-lu merged commit b5e6a19 into FoundationDB:main Jun 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants