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 aggregation memory revoke when operator is finishing #164
Conversation
27089f4
to
4afc33d
Compare
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.
Generally looks good. Please update code as fixups for easier re-review.
...src/main/java/io/prestosql/operator/aggregation/builder/SpillableHashAggregationBuilder.java
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/TestHashAggregationOperator.java
Outdated
Show resolved
Hide resolved
.addSequencePage(10, 100, 0, 300, 0, 500) | ||
.addSequencePage(numberOfRows, 100, 0, 100000, 0, 500) | ||
.addSequencePage(numberOfRows, 100, 0, 200000, 0, 500) | ||
.addSequencePage(numberOfRows, 100, 0, 300000, 0, 500) |
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.
nit: 300_000
presto-main/src/test/java/io/prestosql/operator/TestHashAggregationOperator.java
Show resolved
Hide resolved
|
||
MaterializedResult expected = expectedBuilder.build(); | ||
MaterializedResult actual = toMaterializedResult(driverContext.getSession(), expected.getTypes(), pages); | ||
assertEqualsIgnoreOrder(actual.getMaterializedRows(), expected.getMaterializedRows()); |
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 get it. Previously the code was calling assertOperatorEqualsIgnoreOrder
Now it's more verbose, including manually calling toPages
, dropping hash channels, converting to MaterializedResult
.
i assume this is assertOperatorEqualsIgnoreOrder
inlined, but what's the actual difference?
Is this really related to increasing the data set, as Produce more than single page in testHashAggregation
commit message says?
// have a proper way to atomically convert memory reservations | ||
localRevocableMemoryContext.setBytes(currentRevocableBytes); | ||
// spill since revocable memory could not be converted to user memory immediately | ||
getFutureValue(spillToDisk()); |
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.
This can take time.
Can we spill asynchronously?
spillToDisk();
return WorkProcessor.empty();
(then we need to updateMemory();
on next call..)
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.
Added todo since this requires bigger operator refactor
...src/main/java/io/prestosql/operator/aggregation/builder/SpillableHashAggregationBuilder.java
Show resolved
Hide resolved
...src/main/java/io/prestosql/operator/aggregation/builder/SpillableHashAggregationBuilder.java
Show resolved
Hide resolved
...src/main/java/io/prestosql/operator/aggregation/builder/SpillableHashAggregationBuilder.java
Show resolved
Hide resolved
...src/main/java/io/prestosql/operator/aggregation/builder/SpillableHashAggregationBuilder.java
Show resolved
Hide resolved
4afc33d
to
fdc38d6
Compare
ac |
fdc38d6
to
f2f743f
Compare
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.
nit
presto-main/src/test/java/io/prestosql/operator/TestHashAggregationOperator.java
Show resolved
Hide resolved
f2f743f
to
f7f1701
Compare
…titions to table scan
No description provided.