-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix quadratic runtime in min_max_bytes #18044
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
base: main
Are you sure you want to change the base?
Conversation
78333ab
to
34e6713
Compare
34e6713
to
4d6e5e6
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.
Would you be able to post the results of the benchmark as well, for visibility?
pub mod hash_map { | ||
pub use hashbrown::hash_map::Entry; | ||
} | ||
pub mod hash_set { | ||
pub use hashbrown::hash_set::Entry; | ||
} |
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.
Is this to avoid adding an explicit hashbrown dependency to functions-aggregate?
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.
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
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.
Yeah I guess it makes sense given we already export hashbrown::HashMap
here already 👍
Of course! Please keep in mind that this is a very specific workload and not broader regression testing Benchmark results
|
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.
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
🤖 |
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.
🤖: Benchmark completed Details
|
🤖 |
🤖 |
🤖: Benchmark completed Details
|
🤖 |
🤖 |
🤖: Benchmark completed Details
|
🤖 |
🤖: Benchmark completed Details
|
Which issue does this PR close?
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.