-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Clean up take_native to avoid a panic
#8861
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
Conversation
|
🤖 |
| })) | ||
| .map(|(index, valid)| { | ||
| if valid { | ||
| values[index.as_usize()] |
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.
This also panics the same way? But it is more readable 👍
|
🤖: Benchmark completed Details
|
|
🤖 |
collect_bool to avoid a panictake_native to avoid a panic
Looks a bit slower |
|
🤖: Benchmark completed Details
|
|
Thanks for trying this out! Seems to be a bigger regression than I thought and there are good reasons that the current code is structured that way. |
|
I am also surprised by this too We can probably do better handling nulls 64 bits at a time, but for now I will just drop it as I have plenty of other places to chase performance wins Thanks @jhorstmann and @Dandandan |
Which issue does this PR close?
collect_booland removeunsafe, optimizetake_bits,take_nativefor null values #8849Rationale for this change
@jhorstmann has a suggestion on how to improve this code
collect_booland removeunsafe, optimizetake_bits,take_nativefor null values #8849 (comment)What changes are included in this PR?
Implement said suggestion
Are these changes tested?
By CI and I will run benchmarks
Are there any user-facing changes?
No