feat: merge dictionary values for view types, dedup exactly on overflow - #10927
Open
okhsunrog wants to merge 2 commits into
Open
feat: merge dictionary values for view types, dedup exactly on overflow#10927okhsunrog wants to merge 2 commits into
okhsunrog wants to merge 2 commits into
Conversation
okhsunrog
pushed a commit
to tarantool/arrow-rs
that referenced
this pull request
Aug 31, 2026
Combining `DictionaryArray`s whose dictionaries are built independently (one per partition, per shard, or per record batch) has to merge their values. `merge_dictionary_values` deduplicates, so the merged dictionary only ever holds the distinct referenced values; the `MutableArrayData` fallback in `concat`/`interleave` simply concatenates them and can therefore exceed what the key type addresses even when the distinct values comfortably fit. 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. Add pointer comparison and masked-value extraction for both view types. - 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 still overflowed. Keep the fast path, and on overflow retry the mapping with exact deduplication, which allocates exactly one key per distinct value. With both, merging 16 dictionaries of 60k distinct values under a `UInt16` key yields a 60k-value dictionary instead of failing. Upstream: apache#10927
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. Add pointer comparison and masked-value extraction for both view types. - 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. Keep the fast path, and on overflow retry the mapping with exact deduplication, which allocates exactly one key per distinct value. With both, merging 16 dictionaries of 60k distinct values under a `UInt16` key yields a 60k-value dictionary instead of failing.
okhsunrog
added a commit
to tarantool/arrow-rs
that referenced
this pull request
Aug 31, 2026
Combining `DictionaryArray`s whose dictionaries are built independently (one per partition, per shard, or per record batch) has to merge their values. `merge_dictionary_values` deduplicates, so the merged dictionary only ever holds the distinct referenced values; the `MutableArrayData` fallback in `concat`/`interleave` simply concatenates them and can therefore exceed what the key type addresses even when the distinct values comfortably fit. 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. Add pointer comparison and masked-value extraction for both view types. - 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 still overflowed. Keep the fast path, and on overflow retry the mapping with exact deduplication, which allocates exactly one key per distinct value. With both, merging 16 dictionaries of 60k distinct values under a `UInt16` key yields a 60k-value dictionary instead of failing. Upstream: apache#10927
okhsunrog
force-pushed
the
dict-merge-view-types
branch
from
August 31, 2026 09:47
b0d906e to
47f48a3
Compare
Contributor
|
@okhsunrog I think this is more of a feature than a fix. |
Contributor
Author
I guess we can call it a feature, it depends. What do you suggest? Rename the PR? |
Contributor
feature: support de-duplicating view types in interleave_dictionaries/concat or something like that, just an idea |
Contributor
|
run benchmark interleave_kernels |
Contributor
|
run benchmark concatenate_kernel |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing dict-merge-view-types (47f48a3) to 0aece99 (merge-base) diff Run configurationrun benchmark concatenate_kernelCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Rich-T-kid
suggested changes
Aug 31, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
I took a first pass. this is looking good!
Ill take a closer look at merge_dictionary_values when I take a second look.
|
|
||
| assert_eq!(combined.len(), 400); | ||
| assert_eq!(combined.values().data_type(), &DataType::Utf8View); | ||
| assert!(combined.values().len() < 400); |
Contributor
There was a problem hiding this comment.
Suggested change
| assert!(combined.values().len() < 400); | |
| assert_eq!(combined.values().len(),200); |
Contributor
Author
There was a problem hiding this comment.
It is 251 here, not 200. With only two dictionaries the best-effort interner has enough buckets to do the merge on its own, so the exact retry never runs and 51 duplicates survive. The property worth asserting is that the result fits the key type, so I went with assert!(u8::try_from(combined.values().len()).is_ok()) and said why in a comment. The test still checks every key resolves to its original value, which is the real guarantee.
Comment on lines
+1713
to
+1718
| // 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. |
Contributor
There was a problem hiding this comment.
nit: we should trim this. we dont need the
This mirrors a
Dictionary<UInt16, Utf8View>column read in several partitions, each building its own dictionary, and then combined.
Comment on lines
+1754
to
+1757
| #[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. |
Contributor
There was a problem hiding this comment.
I dont think this is needed. both stringview/binaryview are pretty much the exact same structure. 1 test should be fine
Contributor
Author
There was a problem hiding this comment.
The layouts are the same, but get_masked_values dispatches them through separate arms with separate downcasts:
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.
Contributor
There was a problem hiding this comment.
/// Downcast this to a [`StringViewArray`]
///
/// # Panics
///
/// Panics if this is not a [`StringViewArray`]
fn as_string_view(&self) -> &StringViewArray {
self.as_byte_view_opt().expect("string view array")
}
/// Downcast this to a [`BinaryViewArray`]
///
/// # Panics
///
/// Panics if this is not a [`BinaryViewArray`]
fn as_binary_view(&self) -> &BinaryViewArray {
self.as_byte_view_opt().expect("binary view array")
}these just perform a a downcast so that masked_byte_views() receives a GenericByteViewArray the code paths are identical
arrow-rs/arrow-array/src/array/byte_view_array.rs
Line 1125 in 027b45f
arrow-rs/arrow-array/src/array/byte_view_array.rs
Line 1166 in 027b45f
this isn't too important but i'm in favor or removing it. would be nice to get a third opinion
Comment on lines
+1804
to
+1806
| // Every key is addressable; how far below 200 the merge gets depends on | ||
| // whether the best-effort interner sufficed or the exact retry ran | ||
| assert!(u8::try_from(combined.values().len()).is_ok()); |
Contributor
There was a problem hiding this comment.
Im confused, this always de-dupe the values right? is it possible for only some of the values to be deduplicated? if not
Suggested change
| // Every key is addressable; how far below 200 the merge gets depends on | |
| // whether the best-effort interner sufficed or the exact retry ran | |
| assert!(u8::try_from(combined.values().len()).is_ok()); | |
| // Every key is addressable; how far below 200 the merge gets depends on | |
| // whether the best-effort interner sufficed or the exact retry ran | |
| assert!(u8::try_from(combined.values().len()).is_ok()); | |
| assert_eq!(total_values_len,200) |
Contributor
Author
There was a problem hiding this comment.
Right, this did read badly, the comment was wrong: the merged values can never go below 200, there are exactly that many distinct ones. Four dictionaries do overflow the interner, so the exact retry runs and the count comes out at exactly 200. Asserting that now, with the comment rewritten.
Comment on lines
+145
to
+147
| /// For each referenced value of a dictionary, its index within that dictionary's | ||
| /// values and its bytes (`None` for a null value) | ||
| type MaskedValues<'a> = Vec<(usize, Option<&'a [u8]>)>; |
Contributor
There was a problem hiding this comment.
nice, this is very neat and makes the code easier to follow
Comment on lines
+471
to
+479
| #[cfg_attr(miri, ignore)] // Takes too long | ||
| fn merge_string_view_dictionaries_deduplicates_exactly() { | ||
| // Four dictionaries over the same values: 60000 distinct strings, an | ||
| // empty string and a null. Concatenating them would need 240008 keys, | ||
| // far past the UInt16 range, while the distinct values leave room to | ||
| // spare -- so the merge has to deduplicate them exactly. At this | ||
| // cardinality the best-effort interner alone leaves thousands of | ||
| // duplicates behind and overflows, which forces the exact retry. | ||
| const DISTINCT: usize = 60000; |
Contributor
There was a problem hiding this comment.
I think we can make this test smaller by using u8 key type and having far less distinct values. The same things are being tested but this way this test requires less compute and we dont need to skip the miri check
Contributor
Author
There was a problem hiding this comment.
Good call, done. u8 keys over 200 distinct values still leaves the interner with more duplicates than 256 keys can address, so the exact retry is still the path under test. The miri skip is gone and the test runs in about 14 seconds there.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing dict-merge-view-types (47f48a3) to 0aece99 (merge-base) diff Run configurationrun benchmark interleave_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
okhsunrog
changed the title
Address review feedback on the tests: - Drop the sentence motivating the view-type case from a downstream reader's point of view; the test stands on its own. - Two dictionaries stay within reach of the best-effort interner, so a few duplicates survive the merge and the value count is not exactly 200 there. Assert what actually holds, that the merged values fit the key type. - Four dictionaries leave the interner with more duplicates than the key type can address, so the exact retry runs and the merged values come out one per distinct value. Assert that count exactly. - Shrink `merge_string_view_dictionaries_deduplicates_exactly` to `u8` keys over 200 distinct values. That still overflows the interner and forces the exact retry, while running fast enough under miri that the test no longer has to be skipped there.
Contributor
Author
|
Renamed the PR |
Contributor
|
will take another look in the morning 👍 |
Jefffrey
added
the
enhancement
Contributor
Author
|
@Rich-T-kid could you take a look, please? |
Rich-T-kid
reviewed
Sep 3, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
this PR makes sense to me, but I think we should split this up into two PR because currently its doing two things
- its introducing support for view value types
(utf8View,BinaryView) - its introduces de-duplication to
merge_dictionary_valueswhich it goes about in not the most optimal way.
in terms of support for view types the PR good to go.
As for deduplication I think we need a separate discussion as to why it'd be worth to do when merge_dictionary_values goes out of its way to not do it, as well as benchmarks to gauge any performance degradation.
would be happy to hear your thoughts on this @okhsunrog
Comment on lines
+1754
to
+1757
| #[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. |
Contributor
There was a problem hiding this comment.
/// Downcast this to a [`StringViewArray`]
///
/// # Panics
///
/// Panics if this is not a [`StringViewArray`]
fn as_string_view(&self) -> &StringViewArray {
self.as_byte_view_opt().expect("string view array")
}
/// Downcast this to a [`BinaryViewArray`]
///
/// # Panics
///
/// Panics if this is not a [`BinaryViewArray`]
fn as_binary_view(&self) -> &BinaryViewArray {
self.as_byte_view_opt().expect("binary view array")
}these just perform a a downcast so that masked_byte_views() receives a GenericByteViewArray the code paths are identical
arrow-rs/arrow-array/src/array/byte_view_array.rs
Line 1125 in 027b45f
arrow-rs/arrow-array/src/array/byte_view_array.rs
Line 1166 in 027b45f
this isn't too important but i'm in favor or removing it. would be nice to get a third opinion
Comment on lines
+293
to
+296
| // The duplicates left behind by the interner's hash collisions can push | ||
| // the output past what the key type can address even though the distinct | ||
| // values would have fit. Retry with exact deduplication, which allocates | ||
| // exactly one key per distinct value at the cost of a hash map. |
Contributor
There was a problem hiding this comment.
a bit unrelated to this PR but I think it'd be useful to add some of this info to the Internern::intern(). its easier to understand why we retry on DictionaryKeyOverFlowError if the Internern::intern() was a bit more clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Combining
DictionaryArrays whose dictionaries were built independently has to merge their values.merge_dictionary_valuesdeduplicates, so the merged dictionary only holds the distinct referenced values; theMutableArrayDatafallback inconcat/interleaveconcatenates 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_valuesreturnedfalsefor any value type that is neither primitive nor an offset-based byte array, soDictionary(_, Utf8View)andDictionary(_, BinaryView)always took the non-deduplicating fallback. Reaching the merge path would then have hitunimplemented!()inget_masked_values, which has no arm for the view layouts either. The two are indistinguishable to a caller: identical data merges asUtf8and fails asUtf8View.The
Internerbacking 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 aUInt16key left ~40% duplicates and overflowed anyway. This affectsUtf8dictionaries 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.rsandinterleave.rsgain tests only.should_merge_dictionary_values: compareUtf8View/BinaryViewvalues throughArrayData::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 newmasked_byte_views.merge_dictionary_values: keep the interner fast path, and onDictionaryKeyOverflowErrorretry the key assignment with exact deduplication, which allocates exactly one key per distinct value. The shared loop moves intocompute_key_mappings, parameterised by the key-assignment closure.With both, merging 16 dictionaries of 60k distinct values under a
UInt16key yields a 60k-value dictionary instead of failing.Are these changes tested?
Yes, five new tests:
concat_string_view_dictionary_merges_duplicate_valuesandtest_interleave_string_view_dictionary_merges_duplicate_valuescover the first gap: twoDictionary(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_valuescovers the other view layout.merge_string_view_dictionaries_deduplicates_exactlycovers the second gap deterministically, at a cardinality where the interner alone cannot succeed: fourDictionary(UInt8, Utf8View)arrays over 200 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_arrayscovers four dictionaries whose concatenated values would need 800 keys under aUInt8key.The existing
*_overflow_returns_errtests continue to pin the genuine-overflow behaviour.Are there any user-facing changes?
No API changes. Cases that previously returned
ArrowError::DictionaryKeyOverflowErrormay 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.