-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(rust!): Rename ChunkedArray.try_apply
to try_apply_values
#14947
Conversation
self.try_apply(|t| { | ||
self.try_apply_values(|t| { |
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 function looks expensive enough that it may be worth skipping null values for - with more consistent naming, this potential inefficiency jumps out a bit more
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14947 +/- ##
==========================================
+ Coverage 81.21% 81.22% +0.01%
==========================================
Files 1348 1348
Lines 175318 175332 +14
Branches 2506 2508 +2
==========================================
+ Hits 142390 142422 +32
+ Misses 32448 32431 -17
+ Partials 480 479 -1 ☔ View full report in Codecov by Sentry. |
Thanks, I wan to wait a bit with merging so we can still patch non-breaking rust versions. |
ChunkedArray.try_apply
to try_apply_values
I think this one is named a little misleadingly, as for
apply
there's:ChunkedArray.apply
: run function on non-null valuesChunkedArray.apply_values
: run function on all valuesBut
ChunkedArray.try_apply
runs a fallible function on all valuesI'd suggest renaming it to
try_apply_values
, and then later introducingtry_apply
which runs a fallible function on non-null values (which would be useful in addressing #11579)