okhsunrog opened a new issue, #10674:
URL: https://github.com/apache/arrow-rs/issues/10674
## Describe the bug
`interleave()` and `concat()` panic with `MutableArrayData::new is
infallible: DictionaryKeyOverflowError` when merging `Dictionary<K, Utf8View>`
(or `BinaryView`) arrays whose combined values genuinely exceed the range of
the dictionary key type `K` -- either at the top level, or nested inside a
`List`/`FixedSizeList`/`Struct`/`RunEndEncoded`/`Union`. This happens even
though both functions have a `Result` return type and are documented/expected
to report errors, not panic.
Two separate issues combine to cause this:
1. `should_merge_dictionary_values` (`arrow-select/src/dictionary.rs`) only
has a fast, pointer-equality-based path for `Utf8`, `LargeUtf8`, `Binary`,
`LargeBinary` and primitive dictionary value types. `Utf8View`/`BinaryView`
fall into the generic `dt => { if !dt.is_primitive() { ... } }` branch, which
never checks `should_merge` and computes `has_overflow` from a
**non-deduplicated sum** of all input dictionaries' value lengths.
2. When `has_overflow` is `true`, `interleave()`/`concat()` route to
`interleave_fallback`/`concat_fallback`, which build a `MutableArrayData`
directly from the original (still `Dictionary`-typed) arrays. For
`DataType::Dictionary`, `MutableArrayData::with_capacities` unconditionally
attempts to concatenate the input dictionaries whenever they don't share the
same underlying buffer (`ptr_eq`), and `.expect()`s the result -- so when the
true combined dictionary length exceeds what `K` can address, it panics instead
of returning `Err(ArrowError::DictionaryKeyOverflowError)`. The same recursive
construction is used for dictionaries nested inside container types, so the
panic isn't limited to top-level dictionary arrays.
In other words: for `Utf8View`/`BinaryView` dictionaries specifically, there
is currently **no path** that returns a clean error for genuine key overflow --
it always panics.
This is distinct from #9366 / #10323, which fixed an off-by-one in the
overflow check (256 values with `u8` keys should fit but didn't). Here the
overflow is real (the key type genuinely cannot address that many distinct
values), so the fix isn't to raise the threshold but to surface it as `Err`
instead of a panic, consistent with `interleave`'s documented `Result` return
type.
## To Reproduce
```rust
use std::sync::Arc;
use arrow_array::{DictionaryArray, StringViewArray, UInt8Array};
use arrow_array::types::UInt8Type;
use arrow_select::interleave::interleave;
// Two independently-built `Dictionary<UInt8, Utf8View>` arrays, each within
// the u8 key range on its own, but whose *combined* distinct values
overflow it.
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 indices: Vec<_> = (0..200).flat_map(|i| [(0, i), (1, i)]).collect();
// Panics instead of returning Err(ArrowError::DictionaryKeyOverflowError):
let _ = interleave(&[&dict_a, &dict_b], &indices);
```
```
thread 'main' panicked at arrow-data/src/transform/mod.rs:680:31:
MutableArrayData::new is infallible: DictionaryKeyOverflowError
```
The same panic reproduces with `concat()`, and with the dictionary nested
inside a `FixedSizeList<Dictionary<UInt8, Utf8View>>` (exercising the recursive
child construction in `MutableArrayData::with_capacities` rather than the
top-level dictionary handling).
The same shape panic hits in production when using a `Dictionary<UInt16,
Utf8View>` column (e.g. `65536`-entry key range) and a query engine merges
independently dictionary-encoded batches from multiple sources (parallel
partitions, separate nodes, etc.) whose per-batch dictionaries sum past the key
range, even though the column's true deduplicated cardinality never exceeds it.
`Utf8View` is a common physical type for string-typed dictionary columns in
current Arrow/DataFusion versions, so this is easy to hit unintentionally.
## Expected behavior
`interleave()`/`concat()` should return
`Err(ArrowError::DictionaryKeyOverflowError)` (as they already do for
`Utf8`/`LargeUtf8`/`Binary`/`LargeBinary` dictionaries hitting the same
genuine-overflow condition) instead of panicking, for `Utf8View`/`BinaryView`
dictionaries too, whether at the top level or nested inside a container type.
## Additional context
- Related: #7466 (fixed dictionary merging for primitive value types,
explicitly left byte/view types as a follow-up), #8640 / #9366 / #10323 (fixed
a different, off-by-one overflow check, not the genuine-overflow panic).
- `davidhewitt`'s comment on #7466 flagged this exact gap: "It seems like
the function `merge_dictionary_values` ... would need to be updated to support
other array types."
- I have a fix ready (adds fallible
`MutableArrayData::try_new`/`try_with_capacities`, used recursively for nested
children, and switches `interleave_fallback`/`concat_fallback` to use them) and
will open a PR referencing this issue.
--
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]
