Enable sorting on column with nulls using query-planning - #15639
Conversation
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
Minor request for a little more doc on the differences.
|
|
||
|
|
||
| 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
|
|
||
|
|
||
| @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 |
3 participants
Description
Related to #15027
Checklist