okhsunrog commented on code in PR #10927:
URL: https://github.com/apache/arrow-rs/pull/10927#discussion_r3905591079
##########
arrow-select/src/concat.rs:
##########
@@ -1708,6 +1708,124 @@ mod tests {
assert_eq!(array.logical_null_count(), 10);
}
+ #[test]
+ fn concat_string_view_dictionary_merges_duplicate_values() {
+ // Two independently-built `Dictionary<UInt8, Utf8View>` arrays
holding the
+ // same 200 distinct values. Naively concatenating their dictionaries
yields
+ // 400 entries, which overflows the u8 key range, but the distinct
values do
+ // fit -- so the values must be merged and deduplicated instead. This
mirrors
+ // a `Dictionary<UInt16, Utf8View>` column read in several partitions,
each
+ // building its own dictionary, and then combined.
+ let dict = |offset: usize| {
+ let values: StringViewArray = (0..200).map(|i|
Some(format!("v{i}"))).collect();
+ let keys = UInt8Array::from_iter_values((0..200).map(|i| (i +
offset) as u8 % 200));
+ DictionaryArray::<UInt8Type>::new(keys, Arc::new(values))
+ };
+ let (a, b) = (dict(0), dict(7));
+
+ let combined = concat(&[&a, &b]).unwrap();
+ let combined = combined.as_dictionary::<UInt8Type>();
+
+ assert_eq!(combined.len(), 400);
+ assert_eq!(combined.values().data_type(), &DataType::Utf8View);
+ assert!(combined.values().len() < 400);
+
+ let values = combined.values().as_string_view();
+ let actual: Vec<_> = combined
+ .keys()
+ .values()
+ .iter()
+ .map(|k| values.value(*k as usize))
+ .collect();
+ let expected: Vec<_> = [&a, &b]
+ .iter()
+ .flat_map(|d| {
+ let v = d.values().as_string_view();
+ d.keys()
+ .values()
+ .iter()
+ .map(|k| v.value(*k as usize))
+ .collect::<Vec<_>>()
+ })
+ .collect();
+ assert_eq!(actual, expected);
+ }
+
+ #[test]
+ fn concat_binary_view_dictionary_merges_duplicate_values() {
+ // Same as `concat_string_view_dictionary_merges_duplicate_values`,
for the
+ // other view-typed dictionary value layout.
Review Comment:
The layouts are the same, but `get_masked_values` dispatches them through
separate arms with separate downcasts:
```rust
DataType::Utf8View => masked_byte_views(array.as_string_view(), mask),
DataType::BinaryView => masked_byte_views(array.as_binary_view(), mask),
```
Swap those and `as_binary_view()` panics on a `Utf8View` array, which
nothing else would catch. The test is cheap, so I would rather keep it, but
happy to drop it if you disagree.
--
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]
