kumarUjjawal commented on code in PR #24476:
URL: https://github.com/apache/datafusion/pull/24476#discussion_r3874027199
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs:
##########
@@ -733,6 +770,61 @@ mod tests {
);
}
+ #[test]
+ fn take_n_reuses_leftover_values_and_treats_emitted_values_as_new() {
+ let mut col = utf8_col();
+ col.vectorized_append(
+ &i32_dict(
+ &[Some(0), Some(1), Some(2)],
+ &[Some("a"), Some("b"), Some("c")],
+ ),
+ &[0, 1, 2],
+ )
+ .unwrap();
+ let _ = col.take_n(2);
+
+ let again = i32_dict(&[Some(0), Some(1)], &[Some("c"), Some("a")]);
+ col.vectorized_append(&again, &[0, 1]).unwrap();
+
+ let mut buf = all_true(2);
+ col.vectorized_equal_to(&[0, 1], &again, &[0, 1], &mut buf);
+ assert_eq!(bool_vec(&buf), vec![true, false]);
+
+ let out = Box::new(col).build();
+ assert_eq!(
+ str_values(&out),
+ vec![Some("c".into()), Some("c".into()), Some("a".into())]
+ );
+ }
+
+ #[test]
+ fn take_n_utf8_view_emits_and_interns_leftover_values() {
Review Comment:
This test passes when you delete the `gc_array` call. It asserts on values
and on equality, and `gc()` changes only the buffer layout, not the data. The
three strings also hold about 60 bytes of
non-inline data. `should_gc_view_array` needs more than 10 KB, so the
compaction never runs here. Could the test build enough non-inline data to pass
that limit, then assert on
`get_buffer_memory_size()` of the emitted array?
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs:
##########
@@ -500,6 +509,11 @@ impl<K: ArrowDictionaryKeyType + Send + Sync> GroupColumn
Int64Array::from_iter(emit_new_to_old.iter().map(|&i| i as i64));
let compact_emit_values =
take(&*all_inner_values, &emit_indices, None).expect("take emit
values");
+ // take() keeps original view buffers while spill's gc_array copies
only when worth it
+ let gced_compact_emit_values = gc_array(&compact_emit_values)
Review Comment:
Would it work to add a case with `Utf8View` values longer than twelve bytes,
and enough of them to pass the 10 KB limit?
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs:
##########
@@ -536,28 +552,33 @@ impl<K: ArrowDictionaryKeyType + Send + Sync> GroupColumn
new_to_old.push(old);
}
- self.value_dedup = HashTable::new();
- self.value_dedup_size = 0;
- self.null_inner_slot = None;
-
- self.hash_values(&all_inner_values);
+ let leftover_non_null_len = new_to_old.len() - null_old_slot.is_some()
as usize;
+ let old_hashes = std::mem::take(&mut self.slot_hash);
+ // Remapping entries from current slot
+ self.value_dedup.retain(|(_, slot)| {
Review Comment:
would a `shrink_to_fit` help here, with the accounting corrected to match?
--
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]
