Jefffrey commented on code in PR #10675:
URL: https://github.com/apache/arrow-rs/pull/10675#discussion_r3809459921
##########
arrow-data/src/transform/mod.rs:
##########
@@ -410,6 +410,20 @@ impl<'a> MutableArrayData<'a> {
Self::with_capacities(arrays, use_nulls, Capacities::Array(capacity))
}
+ /// Fallible variant of [MutableArrayData::new].
+ ///
+ /// Unlike [MutableArrayData::new], this does not panic when merging
dictionary
+ /// arrays whose combined values would overflow the dictionary key type.
Instead,
+ /// it returns an error, letting callers (e.g. [`interleave`](crate) /
`concat`)
+ /// surface it as a normal error.
Review Comment:
this is sufficient for this PR, but in a followup we'll likely need to
modify `try_with_capacities()` to return errors for all its other panicking
behaviour and adjust this doc to be less specific toward the dictionary
overflow behaviour
##########
arrow-data/src/transform/mod.rs:
##########
@@ -418,12 +432,29 @@ impl<'a> MutableArrayData<'a> {
/// # Panics
///
/// This function panics if the given `capacities` don't match the data
type
- /// of `arrays`. Or when a [Capacities] variant is not yet supported.
+ /// of `arrays`. Or when a [Capacities] variant is not yet supported. Or
when
+ /// merging dictionary arrays whose combined values overflow the
dictionary key
+ /// type — see [MutableArrayData::try_with_capacities] for a fallible
variant.
Review Comment:
can list as bullet points instead of chaining or's
```rust
/// # Panics
///
/// * if given `capacities` don't match the data type of `arrays`
/// * if a `Capacities` variant is not yet supported
/// * when merging dictionary arrays whose combined values overflow the
dictionary key type
```
##########
arrow-select/src/concat.rs:
##########
@@ -1732,6 +1732,51 @@ mod tests {
);
}
+ #[test]
+ fn concat_string_view_dictionary_overflow_returns_err() {
+ // Two independently-built `Dictionary<UInt8, Utf8View>` arrays, each
within
+ // the u8 key range on its own, but whose *combined* distinct values
overflow
+ // it (analogous to per-partition dictionary-encoded columns being
merged).
+ let values_a: StringViewArray = (0..200).map(|i|
Some(format!("a{i}"))).collect();
+ let keys_a = UInt8Array::from_iter_values(0..200);
+ let dict_a = DictionaryArray::<UInt8Type>::new(keys_a,
Arc::new(values_a));
+
+ let values_b: StringViewArray = (0..200).map(|i|
Some(format!("b{i}"))).collect();
+ let keys_b = UInt8Array::from_iter_values(0..200);
+ let dict_b = DictionaryArray::<UInt8Type>::new(keys_b,
Arc::new(values_b));
+
+ // Must not panic: the key type genuinely cannot address 400 distinct
values,
+ // so this should surface as a normal, catchable error.
+ let err = concat(&[&dict_a, &dict_b]).unwrap_err();
+ assert!(matches!(err, ArrowError::DictionaryKeyOverflowError));
Review Comment:
```suggestion
// concatenating dictionaries which results in overflowing the key
type should
// surface an error not a panic
let values_a: StringViewArray = (0..200).map(|i|
Some(format!("a{i}"))).collect();
let keys_a = UInt8Array::from_iter_values(0..200);
let dict_a = DictionaryArray::<UInt8Type>::new(keys_a,
Arc::new(values_a));
let values_b: StringViewArray = (0..200).map(|i|
Some(format!("b{i}"))).collect();
let keys_b = UInt8Array::from_iter_values(0..200);
let dict_b = DictionaryArray::<UInt8Type>::new(keys_b,
Arc::new(values_b));
let err = concat(&[&dict_a, &dict_b]).unwrap_err();
assert!(matches!(err, ArrowError::DictionaryKeyOverflowError));
```
we can cut down the verbosity of the comments; same for the other tests below
--
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]
