okhsunrog opened a new pull request, #10927: URL: https://github.com/apache/arrow-rs/pull/10927
# Which issue does this PR close? - Closes #10925. # Rationale for this change Combining `DictionaryArray`s whose dictionaries were built independently has to merge their values. `merge_dictionary_values` deduplicates, so the merged dictionary only holds the distinct referenced values; the `MutableArrayData` fallback in `concat`/`interleave` concatenates them and can therefore exceed what the key type addresses even when the distinct values fit it comfortably. Two gaps kept that merge from happening: - `should_merge_dictionary_values` returned `false` for any value type that is neither primitive nor an offset-based byte array, so `Dictionary(_, Utf8View)` and `Dictionary(_, BinaryView)` always took the non-deduplicating fallback. Reaching the merge path would then have hit `unimplemented!()` in `get_masked_values`, which has no arm for the view layouts either. The two are indistinguishable to a caller: identical data merges as `Utf8` and fails as `Utf8View`. - The `Interner` backing the merge is best-effort by design: a hash collision evicts the previous occupant, so one value can be handed several keys. Merging 4 dictionaries of 60k distinct values under a `UInt16` key left ~40% duplicates and overflowed anyway. This affects `Utf8` dictionaries too, it is simply less visible there. # What changes are included in this PR? All of the logic is in `arrow-select/src/dictionary.rs`; `concat.rs` and `interleave.rs` gain tests only. - `should_merge_dictionary_values`: compare `Utf8View`/`BinaryView` values through `ArrayData::ptr_eq`, which covers the views buffer and the data buffers behind it. - `get_masked_values`: extract masked values for both view layouts, through a new `masked_byte_views`. - `merge_dictionary_values`: keep the interner fast path, and on `DictionaryKeyOverflowError` retry the key assignment with exact deduplication, which allocates exactly one key per distinct value. The shared loop moves into `compute_key_mappings`, parameterised by the key-assignment closure. With both, merging 16 dictionaries of 60k distinct values under a `UInt16` key yields a 60k-value dictionary instead of failing. # Are these changes tested? Yes, five new tests: - `concat_string_view_dictionary_merges_duplicate_values` and `test_interleave_string_view_dictionary_merges_duplicate_values` cover the first gap: two `Dictionary(UInt8, Utf8View)` arrays over the same 200 distinct values, which fail to combine without the fix. Both check that every key still resolves to the value it started out with. - `concat_binary_view_dictionary_merges_duplicate_values` covers the other view layout. - `merge_string_view_dictionaries_deduplicates_exactly` covers the second gap deterministically, at a cardinality where the interner alone cannot succeed: four `Dictionary(UInt16, Utf8View)` arrays over 60000 distinct values plus an empty string and a null. It asserts exactly one key per distinct value, that null and empty string stay apart, and that every mapping preserves its value. - `concat_dictionary_merges_values_of_many_arrays` covers four dictionaries whose concatenated values would need 800 keys under a `UInt8` key. The existing `*_overflow_returns_err` tests continue to pin the genuine-overflow behaviour. # Are there any user-facing changes? No API changes. Cases that previously returned `ArrowError::DictionaryKeyOverflowError` may now succeed. A genuine overflow, where there really are more distinct values than the key type can address, still errors — but only after the retry pass has run in full. Dictionaries make no uniqueness guarantee, so there is no cheap way to tell the two apart in advance. -- 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]
