Remove cudf.Scalar from shift/fillna #17922
Conversation
…ue return plc.Scalars
…alar/shift_fillna
mroeschke
added
Python
| @@ -761,13 +760,21 @@ def _check_scatter_key_length( | |||
| 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).
| 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?
| @@ -168,16 +170,35 @@ def _binaryop(self, other: ColumnBinaryOperand, op: str): | |||
|
|
|||
| 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.
Description
Toward #17843
Checklist