-
Notifications
You must be signed in to change notification settings - Fork 855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support building comparator for dictionaries of primitive integer values #2673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the length of columns = 1 case, sort_to_indices
will be called instead and we also need to add same support there. I will add that support in another PR later.
cc @sunchao |
This is needed to extend dictionary support coverage of sorting kernel. |
arrow/src/array/ord.rs
Outdated
@@ -101,6 +126,27 @@ where | |||
}) | |||
} | |||
|
|||
macro_rules! cmp_dict_primitive { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use a method here? seems the only reason is because of returning Err
. I think we can just panic here since the dictionary key type can't be anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to a generic function.
79229f3
to
a7964c8
Compare
Go build has some issue. It should be unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks. |
Benchmark runs are scheduled for baseline = c25d16e and contender = df4906d. df4906d is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2672.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?