-
-
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: Raise informative error instead of panicking when passing invalid directives to to_string
for Date dtype
#17670
Conversation
to_string
for Date dtypeto_string
for Date dtype
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17670 +/- ##
=======================================
Coverage 80.49% 80.49%
=======================================
Files 1503 1503
Lines 197054 197049 -5
Branches 2805 2805
=======================================
- Hits 158615 158613 -2
+ Misses 37918 37915 -3
Partials 521 521 ☔ View full report in Codecov by Sentry. |
to_string
for Date dtypeto_string
for Date dtype
to_string
for Date dtypeto_string
for Date dtype
…ng unsupported directives to `.dt.to_string`
@@ -115,6 +115,55 @@ where | |||
|
|||
ChunkedArray::try_from_chunk_iter(self.name(), iter) | |||
} | |||
|
|||
pub fn apply_to_buffer_generic<'a, F>(&'a self, mut f: F) -> StringChunked |
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.
Except, it shouldn't be called this, as it is not generic. It returns a StringChunked
/
Maybe apply_to_string_amortized
?
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.
true, I was just thinking that, respect to apply_to_buffer
, the input can be any PolarsDataType
I think apply_to_string_amortized
makes it sounds like the input is a String
?
EDIT I guess apply_to_struct
also follows this naming pattern...OK, updating 👍
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.
Would apply_into_string
be an OK name? apply_to_string
to me just reads like the string is the input rather than the output
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.
Sure, but change the apply_to_struct
and other as well then.
.downcast_iter() | ||
.map(|arr| { | ||
let mut mutarr = MutablePlString::with_capacity(arr.len()); | ||
arr.iter().try_for_each(|opt| { |
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.
Can we make this just a loop? It's a bit less cluttered with the fallible return types.
closes #16271
I've added
apply_to_buffer_generic
andtry_apply_to_buffer_generic
too as they'd be very useful to plugin authors (expecting them to manually re-implement this is probably unrealistic, and from tests I've done, allocating on each iteration slows things down by 3-4x)