Rich-T-kid commented on code in PR #26826:
URL: https://github.com/apache/datafusion/pull/26826#discussion_r4507272351
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -298,9 +334,76 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
group_values,
hashes_buffer: Default::default(),
random_state: crate::aggregates::AGGREGATION_HASH_SEED,
+ has_dictionary: !STREAMING && has_dictionary,
+ column_hashes_buffer: Default::default(),
})
}
+ #[cfg(not(feature = "force_hash_collisions"))]
+ fn create_dictionary_hashes(
Review Comment:
Split these benchmark cases into #27246 so this PR stays focused.
@jayzhan211, could you please take a look when you get a chance?
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -298,9 +334,76 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
group_values,
hashes_buffer: Default::default(),
random_state: crate::aggregates::AGGREGATION_HASH_SEED,
+ has_dictionary: !STREAMING && has_dictionary,
+ column_hashes_buffer: Default::default(),
})
}
+ #[cfg(not(feature = "force_hash_collisions"))]
+ fn create_dictionary_hashes(
+ &mut self,
+ cols: &[ArrayRef],
+ hashes: &mut [u64],
+ ) -> Result<()> {
+ let group_unique = self.group_values.len() == 1;
+
+ let use_cache = cols.iter().any(|array| {
+ array.as_any_dictionary_opt().is_some_and(|dictionary| {
+ let values = dictionary.values();
+ values.len() <=
MAX_DICTIONARY_VALUES_PER_ROW.saturating_mul(array.len())
+ && (group_unique || values.len() > array.len())
+ })
+ });
+ if !use_cache {
Review Comment:
Addressed in 4b19df3c. Every column now uses the same per-column hashing
semantics through hash_column, the separate column_hashes_buffer path was
removed, and dictionary_hash_paths_agree_on_existing_groups covers the mixed
cached and uncached paths. The focused regression test passes on the current
head.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
