-
Notifications
You must be signed in to change notification settings - Fork 784
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
Compare dictionary with primitive array in eq_dyn
and neq_dyn
#2533
Conversation
@@ -2173,45 +2270,28 @@ macro_rules! typed_dict_compares { | |||
}}; | |||
} | |||
|
|||
/// Helper function to perform boolean lambda function on values from two dictionary arrays, this | |||
/// version does not attempt to use SIMD explicitly (though the compiler may auto vectorize) | |||
fn compare_dict_op<'a, K, V, F>( |
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.
As TypedDictionaryArray
implements ArrayAccessor
too, we can use compare_op
to replace this compare_dict_op
. So removing it here.
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.
Nice 👍. This will make the codegen explosion worse, but I'm not really sure there is much we can do about that....
Benchmark runs are scheduled for baseline = 4949a3d and contender = 1d5656e. 1d5656e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
let left_iter = left.into_iter(); | ||
let right_iter = right.into_iter(); | ||
|
||
let result = left_iter |
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.
As pointed out by @pvsri #2564 (comment) this change introduces unsoundness, as dictionary indexes aren't always valid. I will have a think about how best to handle this
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.
Should we just call self.values.value
instead of value_unchecked
in ArrayAccessor.value_unchecked
of TypedDictionaryArray
? It might be less performance, though.
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.
No, because that will panic... I'm working on a fix - standby
Which issue does this PR close?
Closes #2535.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?