Rich-T-kid commented on code in PR #26826:
URL: https://github.com/apache/datafusion/pull/26826#discussion_r4507261192
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs:
##########
@@ -598,7 +649,12 @@ impl<K: ArrowDictionaryKeyType + Send + Sync> GroupColumn
self.value_dedup_size = 0;
self.null_inner_slot = None;
- self.hash_values(&all_inner_values);
+ if self.values_are_group_unique {
Review Comment:
Fixed in 4b19df3c. take_n now preserves the remaining group order instead of
moving the NULL slot to the end. I also added both requested regressions:
single_dictionary_direct_mapping_after_emit_first_with_null_group and
single_dictionary_direct_mapping_after_oversized_batches_and_emit_first. The
focused single_dictionary_ test run passes on the current head.
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs:
##########
@@ -331,13 +347,56 @@ impl<K: ArrowDictionaryKeyType + Send + Sync> GroupColumn
Some(val_idx) => {
let dict_values = dict.values();
self.sync_value_cache(dict_values);
- self.find_or_insert_value(dict_values, val_idx,
self.val_hashes[val_idx])?
+ self.insert_value(dict_values, val_idx)?
}
};
self.group_to_inner.push(inner_slot);
self.check_key_overflow()
}
+ fn dictionary_hashes<'a>(
+ &'a mut self,
+ values: &ArrayRef,
+ group_unique: bool,
+ ) -> Option<&'a [u64]> {
+ self.values_are_group_unique = group_unique;
+ self.sync_value_cache(values);
+ Some(&self.val_hashes)
+ }
+
+ fn try_map_dictionary_groups(
+ &self,
+ array: &ArrayRef,
+ groups: &mut Vec<usize>,
+ ) -> bool {
+ if !self.values_are_group_unique {
+ return false;
+ }
+
+ let dictionary = array.as_dictionary::<K>();
+ if !self.is_cached(dictionary.values()) {
+ return false;
+ }
+
+ let keys = dictionary.keys();
+ let raw_keys = keys.values();
+ let nulls = keys.nulls();
+ groups.clear();
+ for row in 0..dictionary.len() {
+ let group = if nulls.is_some_and(|nulls| nulls.is_null(row)) {
+ self.null_inner_slot.unwrap_or(usize::MAX)
+ } else {
+ self.val_to_inner[raw_keys[row].as_usize()]
+ };
+ if group == usize::MAX {
Review Comment:
Added single_dictionary_shared_values_new_key_falls_back in 4b19df3c. It
covers a second batch sharing the values array while introducing a previously
unseen key, and verifies that the direct mapping falls back and creates group
2. The focused test passes on the current head.
##########
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:
Added all requested cases and reran them after the correctness fix at exact
head 4b19df3c: alternating dictionary-only at 100K was 1.30x faster
(https://github.com/apache/datafusion/pull/26826#issuecomment-6724683162);
alternating dictionary plus Int32 was flat within variance
(https://github.com/apache/datafusion/pull/26826#issuecomment-6724683664);
shared dictionary plus Int32 was 1.14x faster at 32K and 1.28x faster at 100K
(https://github.com/apache/datafusion/pull/26826#issuecomment-6724683767); and
a fresh 20-value dictionary per batch was flat
(https://github.com/apache/datafusion/pull/26826#issuecomment-6724684068).
--
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]
