arrow-ord: add support for nested types to partition
#7131
Merged
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
|
cc @tustvold since it seems like you've worked in this code recently |
alamb
reviewed
Feb 15, 2025
There was a problem hiding this comment.
Thank you @asubiotto -- this looks great to me.
I think it just needs one more test and it would be good to merge
| if !v.data_type().is_nested() { | ||
| return Ok(distinct(&v1, &v2)?.values().clone()); | ||
| } | ||
| // Given that we're only comparing values, null ordering in the input or |
There was a problem hiding this comment.
I wonder if using eq would be faster 🤔
https://docs.rs/arrow/latest/arrow/compute/kernels/cmp/fn.eq.html
There was a problem hiding this comment.
Do you mean for non-nested types? eq doesn't support nested types similarly to distinct and given they both shell out to compare_op I don't think there should be much of a perf difference between distinct and eq + mapping nulls to booleans (which would be necessary).
alamb
changed the title
partition
This support is currently incorrectly assumed by `BoundedWindowAggExec`, so partitioning on a nested type (e.g. struct) causes a nested comparison failure on execution. This commit adds a check to use distinct on non-nested types and falls back to using make_comparator on nested types.
asubiotto
force-pushed
the
asubiotto-pnested
branch
from
07a5285 to
536082c
Compare
|
Thanks for the review! |
friendlymatthew
pushed a commit
to friendlymatthew/arrow-rs
that referenced
this pull request
Feb 21, 2025
This support is currently incorrectly assumed by `BoundedWindowAggExec`, so partitioning on a nested type (e.g. struct) causes a nested comparison failure on execution. This commit adds a check to use distinct on non-nested types and falls back to using make_comparator on nested types.
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.
This support is currently incorrectly assumed by
BoundedWindowAggExec, so partitioning on a nested type (e.g. struct) causes a nested comparison failure on execution.This commit adds a check to use distinct on non-nested types and falls back to using make_comparator on nested types.
Which issue does this PR close?
Rationale for this change
Please see #7130 for more in depth explanation and alternatives considered.
What changes are included in this PR?
If statement to use the old path on non-nested types and a fallback path to use
make_comparatorto check for value distinctness.Are there any user-facing changes?
Previously failing use cases are now supported.