Preserve null dictionary values in interleave and concat kernels
#7144
Conversation
This file contains 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
This function internally computes value masks describing which values from input dictionaries should remain in the output. Values never referenced by keys are considered redundant. Null values were considered redundant, but they are now preserved as of this commit. This change is necessary because keys can reference null values. Before this commit, the entries of `MergedDictionaries::key_mappings` corresponding to null values were left unset. This caused `concat` and `interleave` to remap all elements referencing them to whatever value at index 0, producing an erroneous result.
…ls_in_values` This test case passes dictionary arrays containing null values (but no null keys) to `concat`.
…ulls` This test case passes two dictionary arrays each containing null values or keys to `interleave`.
|
Have you run the benchmarks to confirm what impact this has? If it regresses performance one option might be to compute the output null mask from the logical nulls of the inputs |
Addresses `clippy::type-complexity`.
|
@tustvold I tried running Theoretically, this can improve performance because |
|
Here's the result of |
tustvold
approved these changes
Feb 19, 2025
There was a problem hiding this comment.
This looks good to me.
To check my understanding, previously the logic correctly used the logical null mask in order to determine what values to intern, however, this was insufficient as the remapping logic would be unaware of this and blindly remap the keys that referred to the null values to arbitrary new values.
This PR allows the interner to intern a null value, thereby reserving a new key in the output dictionary for this purpose.
friendlymatthew
pushed a commit
to friendlymatthew/arrow-rs
that referenced
this pull request
Feb 21, 2025
…pache#7144) * fix(select): preserve null values in `merge_dictionary_values` This function internally computes value masks describing which values from input dictionaries should remain in the output. Values never referenced by keys are considered redundant. Null values were considered redundant, but they are now preserved as of this commit. This change is necessary because keys can reference null values. Before this commit, the entries of `MergedDictionaries::key_mappings` corresponding to null values were left unset. This caused `concat` and `interleave` to remap all elements referencing them to whatever value at index 0, producing an erroneous result. * test(select): add test case `concat::test_string_dictionary_array_nulls_in_values` This test case passes dictionary arrays containing null values (but no null keys) to `concat`. * test(select): add test case `interleave::test_interleave_dictionary_nulls` This test case passes two dictionary arrays each containing null values or keys to `interleave`. * refactor(select): add type alias for `Interner` bucket Addresses `clippy::type-complexity`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Closes #6302.
Rationale for this change
Fixing a bug.
What changes are included in this PR?
arrow_select::dictionary::merge_dictionary_valuesnow preserves null values from input dictionaries. This is necessary because keys can reference null values. Without this change, the entries ofMergedDictionaries::key_mappingscorresponding to null values would be left unset, causingconcatandinterleaveto remap all elements referencing them to whatever value at index 0, producing an erroneous result.The handling of null keys (physical nulls) is unchanged.
Are there any user-facing changes?