-
Notifications
You must be signed in to change notification settings - Fork 110
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
Conversation
(prefixLength < highBytes.length) && | ||
(lowBytes[prefixLength] == highBytes[prefixLength])) { | ||
(prefixLength < highBytes.length) && | ||
(lowBytes[prefixLength] == highBytes[prefixLength])) { |
There was a problem hiding this comment.
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.
...ayer-core/src/main/java/com/apple/foundationdb/record/cursors/aggregate/AggregateCursor.java
Show resolved
Hide resolved
...ayer-core/src/main/java/com/apple/foundationdb/record/cursors/aggregate/AggregateCursor.java
Show resolved
Hide resolved
} | ||
|
||
@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), |
There was a problem hiding this comment.
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
.
...ayer-core/src/main/java/com/apple/foundationdb/record/cursors/aggregate/AggregateCursor.java
Show resolved
Hide resolved
...ore/src/test/java/com/apple/foundationdb/relational/recordlayer/query/GroupByQueryTests.java
Outdated
Show resolved
Hide resolved
...java/com/apple/foundationdb/record/provider/foundationdb/query/FDBStreamAggregationTest.java
Outdated
Show resolved
Hide resolved
...java/com/apple/foundationdb/record/provider/foundationdb/query/FDBStreamAggregationTest.java
Show resolved
Hide resolved
...java/com/apple/foundationdb/record/provider/foundationdb/query/FDBStreamAggregationTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@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), |
There was a problem hiding this comment.
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?
AggregateCursor.AggregateCursorContinuation
, which contains partialAggregationResult.