Skip to content

Break GroupedHashAggregateStream spill batch into smaller chunks #8004

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

Break GroupedHashAggregateStream spill batch into smaller chunks #8004

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion datafusion/physical-plan/src/aggregates/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2155,7 +2155,7 @@ mod tests {
spill: bool,
) -> Result<()> {
let task_ctx = if spill {
new_spill_ctx(2, 2812)
new_spill_ctx(2, 2886)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, with original value it will panic with ResourcesExhausted in the merge phase, so I have increase it. Did not do in depth analysis of the problem

} else {
Arc::new(TaskContext::default())
};
Expand Down
11 changes: 10 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 @@ -673,7 +673,16 @@ impl GroupedHashAggregateStream {
let spillfile = self.runtime.disk_manager.create_tmp_file("HashAggSpill")?;
let mut writer = IPCWriter::new(spillfile.path(), &emit.schema())?;
// TODO: slice large `sorted` and write to multiple files in parallel
writer.write(&sorted)?;
let mut offset = 0;
let total_rows = sorted.num_rows();

while offset < total_rows {
let length = std::cmp::min(total_rows - offset, self.batch_size);
let batch = sorted.slice(offset, length);
offset += batch.num_rows();
writer.write(&batch)?;
}

Comment on lines 675 to +685
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to write in parallel? So that this becomes less blocking.

Additional improvement would be that chunking before sorting. I remember the discussion was sorted keeps a copy in memory. We can slice emit before sort_batch and sort right before writing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see big benefit of writing in parallel, but we can give it a try at the later date.
Will try to implement some of the alternatives we discuss, at the moment addressing low hanging fruits.

Copy link
Contributor

@kazuyukitanimura kazuyukitanimura Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Got it.
Would you mind explaining how reading smaller files reduces memory usage? I thought we are streaming when reading back for merging. Also, when merge-sort is going on, we need to open all files anyway? Just making sure the original issue statement in #8003

Additionally, I think we have to create a new writer otherwise, we keep appending to the same temp file? So we end up having the same file size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion, not smaller files, smaller batches. We still keep one file per spill method invocation

Previous implementation would just have one batch per file, this change would introduce more than one batch per file. So file size should be similar size to previous implantation, probably even a bit bigger, but that same file will have more than one batch in it.

Streaming merge will open same number of files like before, but it will load smaller batches into memory (if we have one batch per file it would mean whole file would be loaded).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for explaining!

writer.finish()?;
self.spill_state.spills.push(spillfile);
Ok(())
Expand Down