Enable sorting on column with nulls using query-planning #15639
Conversation
|
|
||
|
|
||
| def _cudf_layer(self): | ||
| if hasattr(self._meta, "to_pandas"): |
There was a problem hiding this comment.
nit: this canary for whether or not we have a cudf-backed object is also true for pyarrow objects I think. I know this is a hypothetical right now, but just noting it.
There was a problem hiding this comment.
Could you add a short comment about what exactly this code does differently from the upstream version such that it can handle nulls?
There was a problem hiding this comment.
nit: this canary for whether or not we have a cudf-backed object is also true for pyarrow objects I think. I know this is a hypothetical right now, but just noting it.
Yeah, sorry. This was used as a simple toggle when I was experimenting in the dask-expr code itself. If the code lives in dask-cudf, we can do an instance check.
Could you add a short comment about what exactly this code does differently from the upstream version such that it can handle nulls?
Yes. I agree that a more-thorough comment is definitely needed here.
rjzamora
changed the title
RepartitionQuantiles in dask-expr
| @@ -72,7 +72,7 @@ def test_sort_repartition(): | |||
| dd.assert_eq(len(new_ddf), len(ddf)) | |||
|
|
|||
|
|
|||
| @xfail_dask_expr("dask-expr code path fails with nulls") | |||
| @xfail_dask_expr("missing null support", lt_version="2024.5.1") | |||
There was a problem hiding this comment.
I'd like this test to run for anything newer than the "2024.5.0" release (including main). Not sure of the "correct" way to do this.
There was a problem hiding this comment.
I think that is correct. pytest.mark.xfail(some_condition, reason="some_reason") which is what this expands to is the canonical way.
rjzamora
added
5 - Ready to Merge
|
/merge |
Description
Related to #15027
Checklist