-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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!: Split replace
functionality into two separate methods
#16921
Conversation
CodSpeed Performance ReportMerging #16921 will not alter performanceComparing Summary
|
bf621f6
to
a6bccde
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16921 +/- ##
==========================================
+ Coverage 80.86% 80.88% +0.02%
==========================================
Files 1456 1456
Lines 191141 191297 +156
Branches 2728 2734 +6
==========================================
+ Hits 154562 154727 +165
+ Misses 36073 36064 -9
Partials 506 506 ☔ View full report in Codecov by Sentry. |
dadc13f
to
aeacbf8
Compare
eee1292
to
2dc0d1f
Compare
@ritchie46 This PR is now ready for review. Design is a bit different from what we discussed in the office:
|
} else { | ||
self.apply_many_private(FunctionExpr::Replace { return_dtype }, &args, false, false) | ||
self.apply_many_private( |
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.
Not for this PR, but I think we can set this to map
later. I don't believe this is group-aware. If the input is group-aware, that would already bubble up that info.
let mut out = new.new_from_index(0, s.len()); | ||
|
||
// Transfer validity from `mask` to `out`. | ||
if mask.null_count() > 0 { |
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 a future PR we must optimize this. Adding a validity is almost free, but here it takes a very expensive branchy path.
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.
Yeah I wasn't happy about this either - but I couldn't find a way to do this without a lot of code... I figure we're probably missing some API building blocks 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.
There is still a few things left to optimize, but let's ensure we have the correct behavior for 1.0 first. 👍
replace
functionality into two separate functionsreplace
functionality into two separate methods
Closes #14302
Changes
replace_strict
method toExpr
andSeries
. This method is similar to whatreplace
was previously, with the exception that it raises an error if any non-null values were not replaced. Specify adefault
to set all values that were not replaced to that default.replace
now always keeps the existing data type - this is a breaking change (see example below). Thedefault
andreturn_dtype
parameters are deprecated.Example
Before:
After: