Skip to content

GroupedHashAggregateStream should register spillable consumer #8002

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.

Sign up for GitHub

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 1 commit into from
Nov 1, 2023
Merged

GroupedHashAggregateStream should register spillable consumer #8002

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion datafusion/physical-plan/src/aggregates/row_hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,9 @@ impl GroupedHashAggregateStream {
.collect();

let name = format!("GroupedHashAggregateStream[{partition}]");
let reservation = MemoryConsumer::new(name).register(context.memory_pool());
let reservation = MemoryConsumer::new(name)
Copy link
Contributor

@alamb alamb Oct 31, 2023

Choose a reason for hiding this comment

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

https://docs.rs/datafusion/latest/datafusion/execution/memory_pool/struct.MemoryConsumer.html#method.with_can_spill

I agree

@kazuyukitanimura and @yjshen do you concur? Is there any way we could potentially test this behavior?

Copy link
Contributor Author

@milenkovicm milenkovicm Oct 31, 2023

Choose a reason for hiding this comment

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

Can we create TestMemoryPool which will panic if unspillable operator is registered ?

Copy link
Contributor

@kazuyukitanimura kazuyukitanimura Oct 31, 2023

Choose a reason for hiding this comment

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

Yes, looks good.
Another idea to test is that to write a test aggregation job with small memory pool size that fails with the greedy pool but succeeds with the fair pool. Not sure if we can write such case...

.with_can_spill(true)
.register(context.memory_pool());

let group_ordering = agg
.aggregation_ordering
Expand Down