Remove cudf.Scalar from shift/fillna - #17922
Conversation
…ue return plc.Scalars
…alar/shift_fillna
mroeschke
added
Python
| f"{num_keys}" | ||
| ) | ||
|
|
||
| def _scalar_to_plc_scalar(self, scalar: ScalarLike) -> plc.Scalar: |
There was a problem hiding this comment.
This feels a bit odd as a class method. I feel like a free function that accepts a dtype would be more appropriate, then we could call that with col.dtype. Scoping-wise this doesn't feel like a Column method. Plus then it would directly mirror pa_scalar_to_plc_scalar.
There was a problem hiding this comment.
I guess there is currently a small benefit because we can override this method for decimal columns to get the specialized behavior that we need, but I think that we don't need that any more (see my comment on that class).
There was a problem hiding this comment.
Now that I've closes #18035 as an attempt to avoid this decimal special casing, do you still feel strongly about having this as a free function? I chose a class method because, as you mentioned, I am able to customize this for decimal and it's a little more obvious when I could remove this in the future
There was a problem hiding this comment.
No, I think it's fine to leave it as is for now.
| isinstance(fill_value, np.datetime64) | ||
| and self.time_unit != np.datetime_data(fill_value)[0] | ||
| ): | ||
| # TODO: Disallow this cast |
There was a problem hiding this comment.
I feel like a lot of your PRs have had these kinds of comments. Do they all fall into similar buckets? Should we open some issues for tracking?
There was a problem hiding this comment.
Actually I double checked pandas and our casts here matches the pandas behavior for this method (although I'm not fond of it)
There was a problem hiding this comment.
The question about tracking TODOs still applies but I'm good with holding off on that. grepping the codebase for these small things is OK with me for now given how much we're churning internally anyway.
|
|
||
| return result | ||
|
|
||
| def _scalar_to_plc_scalar(self, scalar: ScalarLike) -> plc.Scalar: |
There was a problem hiding this comment.
Now that #17422 is merged I think we can stop special-casing this and see if anything breaks. WDYT? It does mean that decimal conversions in tests will fail if run with an older version of pyarrow, but I think that's an OK tradeoff. We might have to put some conditional xfails into our test suite for the "oldest" test runs.
There was a problem hiding this comment.
I opened #18035 to dedicate to avoid the decimal special casing.
Can discuss on that PR, but IIUC, to avoid these conversion on the Python side, we would need pyarrow APIs introduced in pyarrow 19
There was a problem hiding this comment.
Good call. Let's discuss there, I responded in #18035 (comment)
|
Approving since we're putting a pin in #18035. |
|
/merge |
1 similar comment
|
/merge |
2 participants
Description
Toward #17843
Checklist