Require dtype argument to cudf_polars Column container
#19193
Conversation
…olars/struct_expr
…olars/struct_expr
…olars/struct_expr
…eschke/cudf into feat/cudf_polars/struct_expr
…olars/struct_expr
…olars/struct_expr
…olars/struct_expr
…olars/struct_expr
mroeschke
added
improvement
github-actions
bot
added
Python
| if dtype_str.startswith("list["): | ||
| stripped = dtype_str.removeprefix("list[").removesuffix("]") | ||
| return pl.List(_dtype_short_repr_to_dtype(stripped)) | ||
| return pl.datatypes.convert.dtype_short_repr_to_dtype(dtype_str) |
There was a problem hiding this comment.
Question about the return type here: I see that https://github.com/pola-rs/polars/blob/405c371c41d71e4463829062ba297e3378cfd85d/py-polars/polars/datatypes/convert.py returns a PolarsDataType | None.
- How should we handle the
Nonecase (which seems to happen for invalid data types)? PolarsTypeis defined asUnion["DataTypeClass", "DataType"]. Do we need to worry about theDataTypeClassvariant ? I'm not exactly sure what this is.
There was a problem hiding this comment.
How should we handle the None case (which seems to happen for invalid data types)?
If we allow Columns to have an optional, None, dtype I suppose a None return here could still be valid, but that would hide the root cause of an invalid data type
I suppose we can raise/add an assert here, but would it be better to do that during deserialization or before serialization?
Do we need to worry about the DataTypeClass variant ?
Good catch. Yes, it appears this method can return a polars.DataType class (DataTypeClass) and not necessarily an instance which is what our DataType container expects.
There was a problem hiding this comment.
I suppose we can raise/add an assert here, but would it be better to do that during deserialization or before serialization?
I think raising an error here (in the deserialization) is appropriate. If we're unable to parse a dtype then presumably something has gone very wrong.
| pl_type = pl.datatypes.convert.dtype_short_repr_to_dtype(dtype_str) | ||
| if pl_type is None: | ||
| raise ValueError(f"{dtype_str} was not able to be parsed by Polars.") | ||
| return pl_type() if inspect.isclass(pl_type) else pl_type |
There was a problem hiding this comment.
How safe is pl_type(), without any arguments, here? Some types (Array, Enum) require additional arguments. Maybe we don't support those yet?
There was a problem hiding this comment.
For the types we support in DataType, I believe this is fairly safe as I'm hoping that dtype_short_repr_to_dtype will return instances for types with parameters (polars.Datetime and polars.Duration).
For those types that we don't support that take arguments, those should be rejected when constructing a DataType
|
/merge |
With #19193 and this PR, we'll not import `pyarrow` explicitly in `cudf_polars` xref #18534 Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Matthew Murray (https://github.com/Matt711) URL: #19219
Description
Depends on #19075
Following #19091, this PR ensure the
Columnalways contains aDataTypeobject such that Polars type metadata such as struct field names are preservedChecklist