Fix quadratic runtime in min_max_bytes#18044
Merged
alamb merged 4 commits intoapache:mainfrom Oct 19, 2025
Merged
Conversation
github-actions
bot
added
common
ctsk
force-pushed
the
fix/min-max-bytes
branch
from
78333ab to
34e6713
Compare
ctsk
force-pushed
the
fix/min-max-bytes
branch
from
34e6713 to
4d6e5e6
Compare
Jefffrey
reviewed
Oct 15, 2025
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Would you be able to post the results of the benchmark as well, for visibility?
Comment on lines
+111
to
+116
| pub mod hash_map { | ||
| pub use hashbrown::hash_map::Entry; | ||
| } | ||
| pub mod hash_set { | ||
| pub use hashbrown::hash_set::Entry; | ||
| } |
Contributor
There was a problem hiding this comment.
Is this to avoid adding an explicit hashbrown dependency to functions-aggregate?
Contributor
Author
There was a problem hiding this comment.
Yes, the datafusion_common::HashMap::entry API was not usable without adding an explicit dependency on the hashbrown crate, at which point there is little benefit over using hashbrown::HashMap directly
Contributor
There was a problem hiding this comment.
Yeah I guess it makes sense given we already export hashbrown::HashMap here already 👍
Contributor
Author
Of course! Please keep in mind that this is a very specific workload and not broader regression testing Benchmark results |
Jefffrey
approved these changes
Oct 17, 2025
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Makes sense to me; we're just allocating for only the groups we need instead of for all groups (even if not being used in the current update_batch) if I understand correctly
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
Very nice! |
Contributor
|
Thanks again @ctsk |
alamb
reviewed
Oct 19, 2025
| // in this batch (either the existing min/max or the new input value) | ||
| // and updating the owned values in `self.min_maxes` at most once | ||
| let mut locations = vec![MinMaxLocation::ExistingMinMax; total_num_groups]; | ||
| let mut locations = HashMap::<usize, &[u8]>::with_capacity(group_indices.len()); |
Contributor
There was a problem hiding this comment.
i tried a few different thing to avoid having to allocate a HashMap -- like ideally we could at least reuse the allocation from invocation to invocation
However, as it is setup now it has a slice into the input array so any structure ends up with a lifetime tied to the input, which I couldn't figure out how to reuse across calls to different input 🤔
tobixdev
pushed a commit
to tobixdev/datafusion
that referenced
this pull request
Nov 2, 2025
## Which issue does this PR close? - Closes apache#17897 ## What changes are included in this PR? This PR replaces the `locations` vector used to reduce the number of allocations / resizes in the accumulator with. a HashMap instead. ## Are these changes tested? Not in particular. Additional unit-tests and broader regression testing would be useful. A microbenchmark verifies that the runtime is no longer quadratic. ## Are there any user-facing changes? No. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
EeshanBembi
pushed a commit
to EeshanBembi/datafusion
that referenced
this pull request
Nov 24, 2025
## Which issue does this PR close? - Closes apache#17897 ## What changes are included in this PR? This PR replaces the `locations` vector used to reduce the number of allocations / resizes in the accumulator with. a HashMap instead. ## Are these changes tested? Not in particular. Additional unit-tests and broader regression testing would be useful. A microbenchmark verifies that the runtime is no longer quadratic. ## Are there any user-facing changes? No. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
What changes are included in this PR?
This PR replaces the
locationsvector used to reduce the number of allocations / resizes in the accumulator with. a HashMap instead.Are these changes tested?
Not in particular. Additional unit-tests and broader regression testing would be useful. A microbenchmark verifies that the runtime is no longer quadratic.
Are there any user-facing changes?
No.