comphead commented on code in PR #7569:
URL: https://github.com/apache/datafusion-comet/pull/7569#discussion_r4532981099
##########
native/spark-expr/src/hash_funcs/murmur3.rs:
##########
@@ -1148,4 +1148,202 @@ mod tests {
"a dictionary child must keep the per-element path"
);
}
+
+ /// `create_hashes_internal!` specialises maps of (Utf8, Int32), (Int32,
Utf8), (Utf8, Utf8)
+ /// and (Int32, Int32), each with a loop for input without nulls and a
null-aware loop. Covers
+ /// both loops of every pair for both hashes. The null shape has a null
map that still owns an
+ /// entry, which must not be hashed, and a null value, which is skipped
while its key is not.
+ /// Expected values are Spark 4.1.3's `hash` and `xxhash64` (seed 42) on
the same rows.
+ #[test]
+ fn test_specialised_map_pairs_match_spark() {
+ use crate::hash_funcs::create_xxhash64_hashes;
+ use arrow::array::{Array, MapArray, StructArray};
+ use arrow::buffer::{NullBuffer, OffsetBuffer};
+ use arrow::datatypes::{DataType, Field, Fields};
+
+ const STR_KEYS: [&str; 7] = ["a", "b", "", "日本", "x", "yz", "hidden"];
+ const INT_KEYS: [i32; 7] = [1, -1, 0, i32::MAX, i32::MIN, 7, 99];
+ const STR_VALUES: [&str; 7] = ["v1", "", "é", "w", "long value", "z",
"hidden"];
+ const INT_VALUES: [i32; 7] = [10, -20, 0, i32::MIN, i32::MAX, 3, 99];
+
+ // The first `len` entries of a pool, with entry `null_at` null.
+ fn column(
+ utf8: bool,
+ strs: &[&str],
+ ints: &[i32],
+ len: usize,
+ null_at: Option<usize>,
+ ) -> ArrayRef {
+ let valid = |i: usize| Some(i) != null_at;
+ if utf8 {
+ Arc::new(
+ (0..len)
+ .map(|i| valid(i).then(|| strs[i]))
+ .collect::<StringArray>(),
+ )
+ } else {
+ Arc::new(
+ (0..len)
+ .map(|i| valid(i).then(|| ints[i]))
+ .collect::<Int32Array>(),
+ )
+ }
+ }
+
+ fn map(key_utf8: bool, value_utf8: bool, shape: Shape) -> ArrayRef {
+ let (lengths, valid, null_value) = shape;
+ let len = lengths.iter().sum();
+ let keys = column(key_utf8, &STR_KEYS, &INT_KEYS, len, None);
+ let values = column(value_utf8, &STR_VALUES, &INT_VALUES, len,
null_value);
+ let entry_fields = Fields::from(vec![
+ Field::new("key", keys.data_type().clone(), false),
+ Field::new("value", values.data_type().clone(), true),
+ ]);
+ let entries = StructArray::new(entry_fields.clone(), vec![keys,
values], None);
+ Arc::new(MapArray::new(
+ Arc::new(Field::new("entries", DataType::Struct(entry_fields),
false)),
+ OffsetBuffer::from_lengths(lengths.iter().copied()),
+ entries,
+ valid
+ .contains(&false)
+ .then(|| NullBuffer::from(valid.to_vec())),
+ false,
+ ))
+ }
+
+ // Entries per row, map validity per row, and the entry whose value is
null.
+ type Shape = (&'static [usize], &'static [bool], Option<usize>);
+ const NO_NULLS: Shape = (&[2, 0, 1, 3], &[true; 4], None);
+ const WITH_NULLS: Shape = (&[2, 0, 1, 1, 3], &[true, true, false,
true, true], Some(3));
+
+ // (pair, key is Utf8, value is Utf8, [murmur3, xxhash64] without
nulls, the same with nulls)
+ #[allow(clippy::type_complexity)]
+ let cases: [(&str, bool, bool, (Vec<u32>, Vec<u64>), (Vec<u32>,
Vec<u64>)); 4] = [
Review Comment:
This adds about 100 lines of hardcoded murmur3 and xxhash64 constants. Most
of these shapes (all four key/value pairs, no nulls, and a null value whose key
is still hashed) can be built in SQL, and `expressions/hash/hash.sql` already
sets `spark.sql.legacy.allowHashOnMapType=true` and compares `hash` and
`xxhash64` with Spark at run time. Could they go there instead, so no constants
need to be kept in sync? Only the null map row that still owns an entry cannot
come from SQL, and that case can be checked here without constants by comparing
with the same rows with that map row removed. I have not checked that a Parquet
map column reaches the null-free fast loop, so that is worth confirming.
##########
native/spark-expr/src/hash_funcs/murmur3.rs:
##########
@@ -1148,4 +1148,202 @@ mod tests {
"a dictionary child must keep the per-element path"
);
}
+
+ /// `create_hashes_internal!` specialises maps of (Utf8, Int32), (Int32,
Utf8), (Utf8, Utf8)
+ /// and (Int32, Int32), each with a loop for input without nulls and a
null-aware loop. Covers
+ /// both loops of every pair for both hashes. The null shape has a null
map that still owns an
+ /// entry, which must not be hashed, and a null value, which is skipped
while its key is not.
+ /// Expected values are Spark 4.1.3's `hash` and `xxhash64` (seed 42) on
the same rows.
+ #[test]
+ fn test_specialised_map_pairs_match_spark() {
+ use crate::hash_funcs::create_xxhash64_hashes;
+ use arrow::array::{Array, MapArray, StructArray};
+ use arrow::buffer::{NullBuffer, OffsetBuffer};
+ use arrow::datatypes::{DataType, Field, Fields};
+
+ const STR_KEYS: [&str; 7] = ["a", "b", "", "日本", "x", "yz", "hidden"];
+ const INT_KEYS: [i32; 7] = [1, -1, 0, i32::MAX, i32::MIN, 7, 99];
+ const STR_VALUES: [&str; 7] = ["v1", "", "é", "w", "long value", "z",
"hidden"];
+ const INT_VALUES: [i32; 7] = [10, -20, 0, i32::MIN, i32::MAX, 3, 99];
+
+ // The first `len` entries of a pool, with entry `null_at` null.
+ fn column(
+ utf8: bool,
+ strs: &[&str],
+ ints: &[i32],
+ len: usize,
+ null_at: Option<usize>,
+ ) -> ArrayRef {
+ let valid = |i: usize| Some(i) != null_at;
+ if utf8 {
+ Arc::new(
+ (0..len)
+ .map(|i| valid(i).then(|| strs[i]))
+ .collect::<StringArray>(),
+ )
+ } else {
+ Arc::new(
+ (0..len)
+ .map(|i| valid(i).then(|| ints[i]))
+ .collect::<Int32Array>(),
+ )
+ }
+ }
+
+ fn map(key_utf8: bool, value_utf8: bool, shape: Shape) -> ArrayRef {
+ let (lengths, valid, null_value) = shape;
+ let len = lengths.iter().sum();
+ let keys = column(key_utf8, &STR_KEYS, &INT_KEYS, len, None);
+ let values = column(value_utf8, &STR_VALUES, &INT_VALUES, len,
null_value);
+ let entry_fields = Fields::from(vec![
+ Field::new("key", keys.data_type().clone(), false),
+ Field::new("value", values.data_type().clone(), true),
+ ]);
+ let entries = StructArray::new(entry_fields.clone(), vec![keys,
values], None);
+ Arc::new(MapArray::new(
+ Arc::new(Field::new("entries", DataType::Struct(entry_fields),
false)),
+ OffsetBuffer::from_lengths(lengths.iter().copied()),
+ entries,
+ valid
+ .contains(&false)
+ .then(|| NullBuffer::from(valid.to_vec())),
+ false,
+ ))
+ }
+
+ // Entries per row, map validity per row, and the entry whose value is
null.
+ type Shape = (&'static [usize], &'static [bool], Option<usize>);
+ const NO_NULLS: Shape = (&[2, 0, 1, 3], &[true; 4], None);
+ const WITH_NULLS: Shape = (&[2, 0, 1, 1, 3], &[true, true, false,
true, true], Some(3));
Review Comment:
Could the test also hash a sliced map, for example `array.slice(1, n)`, and
compare with the tail of the expected vector? A sliced `MapArray` has offsets
that do not start at 0 while `keys()` and `values()` stay unsliced, which is
exactly what the macro's `$offsets[row_idx]` indexing depends on, and no map
hash test covers it today.
--
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]
