Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality to apply Dtype metadata to ColumnBase #8373

Merged
merged 19 commits into from
Jun 15, 2021
Merged

Add functionality to apply Dtype metadata to ColumnBase #8373

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
918c8c5
Add _apply_type_metadata() to ColumnBase
charlesbluca May 26, 2021
1a061e6
Return self if nothing needs to be done
charlesbluca May 26, 2021
3c470ae
Use apply function in _copy_type_metadata()
charlesbluca May 26, 2021
b7329bd
Remove arrow dtype conversion for now
charlesbluca May 26, 2021
fed7075
Use apply function for arrow copy
charlesbluca May 27, 2021
d6dccbc
Add tests for arrow array conversion
charlesbluca May 27, 2021
a93084b
Merge remote-tracking branch 'upstream/branch-21.08' into apply-type-…
charlesbluca May 27, 2021
7739a25
Add arrow StructArray test
charlesbluca May 27, 2021
27290cb
Reuse computed cudf dtype
charlesbluca May 27, 2021
dc706bf
Migrate _apply_type_metadata to column subclasses
charlesbluca May 28, 2021
e04f052
Handle recursive calls in struct/list methods, consolidate copy method
charlesbluca May 28, 2021
ba5d067
Add build functions for struct/list columns
charlesbluca Jun 1, 2021
ec50e1b
Fix circular import
charlesbluca Jun 1, 2021
cbe027d
Rename apply function
charlesbluca Jun 1, 2021
9e26743
Use utils function for arrow type conversion
charlesbluca Jun 1, 2021
d01f558
Remove ColumnBase copy type functions
charlesbluca Jun 3, 2021
9139229
Return new columns directly
charlesbluca Jun 3, 2021
7f09ab0
Add fix for ListColumn.base_size
charlesbluca Jun 4, 2021
ad80721
Merge remote-tracking branch 'upstream/branch-21.08' into apply-type-…
charlesbluca Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 62 additions & 25 deletions python/cudf/cudf/core/column/column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from cudf.core.buffer import Buffer
from cudf.core.dtypes import (
CategoricalDtype,
Decimal64Dtype,
IntervalDtype,
ListDtype,
StructDtype,
Expand Down Expand Up @@ -1267,6 +1268,32 @@ def scatter_to_table(
}
)

def _apply_type_metadata(self: ColumnBase, dtype: Dtype) -> ColumnBase:
charlesbluca marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(dtype, CategoricalDtype) and not (
isinstance(self, cudf.core.column.CategoricalColumn)
):
self = build_categorical_column(
categories=dtype.categories._values,
codes=as_column(self.base_data, dtype=self.dtype),
mask=self.base_mask,
ordered=dtype.ordered,
size=self.size,
offset=self.offset,
null_count=self.null_count,
)

if isinstance(dtype, StructDtype) and isinstance(
self, cudf.core.column.StructColumn
):
self = self._rename_fields(dtype.fields.keys())

if isinstance(dtype, Decimal64Dtype) and isinstance(
self, cudf.core.column.DecimalColumn
):
self.dtype.precision = dtype.precision

return self

def _copy_type_metadata(self: ColumnBase, other: ColumnBase) -> ColumnBase:
"""
Copies type metadata from self onto other, returning a new column.
Expand All @@ -1276,6 +1303,8 @@ def _copy_type_metadata(self: ColumnBase, other: ColumnBase) -> ColumnBase:
and the children of `other`.
* if none of the above, return `other` without any changes
"""
other = other._apply_type_metadata(self.dtype)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method, along with _copy_type_metadata_from_arrow, is now a one-liner since all the logic is handled in apply. Is it worth it to keep the copy functions, or should we just replace all their appearances with the corresponding apply method call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are really the same thing I'd vote to remove the old ones, less clutter, less confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to avoid the indirection and just delete the methods.


# TODO: This logic should probably be moved to a common nested column
# class.
if isinstance(other, type(self)):
Expand Down Expand Up @@ -2200,6 +2229,17 @@ def full(size: int, fill_value: ScalarLike, dtype: Dtype = None) -> ColumnBase:
return ColumnBase.from_scalar(cudf.Scalar(fill_value, dtype), size)


def _cudf_dtype_from_arrow_type(arrow_type: Dtype) -> Dtype:
if pa.types.is_decimal(arrow_type):
return Decimal64Dtype.from_arrow(arrow_type)
elif pa.types.is_struct(arrow_type):
charlesbluca marked this conversation as resolved.
Show resolved Hide resolved
return StructDtype.from_arrow(arrow_type)
elif pa.types.is_list(arrow_type):
return ListDtype.from_arrow(arrow_type)

return arrow_type


def _copy_type_metadata_from_arrow(
arrow_array: pa.array, cudf_column: ColumnBase
) -> ColumnBase:
Expand All @@ -2211,13 +2251,10 @@ def _copy_type_metadata_from_arrow(
* When `arrow_array` is decimal type and `cudf_column` is
Decimal64Dtype, copy precisions.
"""
if pa.types.is_decimal(arrow_array.type) and isinstance(
cudf_column, cudf.core.column.DecimalColumn
):
cudf_column.dtype.precision = arrow_array.type.precision
elif pa.types.is_struct(arrow_array.type) and isinstance(
cudf_column, cudf.core.column.StructColumn
):
cudf_dtype = _cudf_dtype_from_arrow_type(arrow_array.type)
cudf_column = cudf_column._apply_type_metadata(cudf_dtype)
charlesbluca marked this conversation as resolved.
Show resolved Hide resolved

if isinstance(cudf_column, cudf.core.column.StructColumn):
base_children = tuple(
_copy_type_metadata_from_arrow(arrow_array.field(i), col_child)
for i, col_child in enumerate(cudf_column.base_children)
Expand All @@ -2226,30 +2263,30 @@ def _copy_type_metadata_from_arrow(
return cudf.core.column.StructColumn(
data=None,
size=cudf_column.base_size,
dtype=StructDtype.from_arrow(arrow_array.type),
dtype=cudf_dtype,
mask=cudf_column.base_mask,
offset=cudf_column.offset,
null_count=cudf_column.null_count,
children=base_children,
)
elif pa.types.is_list(arrow_array.type) and isinstance(
cudf_column, cudf.core.column.ListColumn

elif isinstance(cudf_column, cudf.core.column.ListColumn) and (
arrow_array.values and cudf_column.base_children
):
if arrow_array.values and cudf_column.base_children:
base_children = (
cudf_column.base_children[0],
_copy_type_metadata_from_arrow(
arrow_array.values, cudf_column.base_children[1]
),
)
return cudf.core.column.ListColumn(
size=cudf_column.base_size,
dtype=ListDtype.from_arrow(arrow_array.type),
mask=cudf_column.base_mask,
offset=cudf_column.offset,
null_count=cudf_column.null_count,
children=base_children,
)
base_children = (
cudf_column.base_children[0],
_copy_type_metadata_from_arrow(
arrow_array.values, cudf_column.base_children[1]
),
)
return cudf.core.column.ListColumn(
size=cudf_column.base_size,
dtype=cudf_dtype,
mask=cudf_column.base_mask,
offset=cudf_column.offset,
null_count=cudf_column.null_count,
children=base_children,
)

return cudf_column

Expand Down
26 changes: 26 additions & 0 deletions python/cudf/cudf/tests/test_column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,32 @@ def test_as_column_buffer(data, expected):
assert_eq(cudf.Series(actual_column), cudf.Series(expected))


@pytest.mark.parametrize(
"data,expected",
[
(
pa.array([100, 200, 300], type=pa.decimal128(3)),
cudf.core.column.as_column(
[100, 200, 300], dtype=cudf.core.dtypes.Decimal64Dtype(3, 0)
),
),
(
pa.array([{"a": 1, "b": 3}, {"c": 2, "d": 4}]),
cudf.core.column.as_column([{"a": 1, "b": 3}, {"c": 2, "d": 4}]),
),
(
pa.array([[[1, 2, 3], [4, 5, 6]], [[7, 8, 9], [10, 11, 12]]]),
cudf.core.column.as_column(
[[[1, 2, 3], [4, 5, 6]], [[7, 8, 9], [10, 11, 12]]]
),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test gave a somewhat cryptic warning:

cudf/tests/test_column.py::test_as_column_arrow_array[data2-expected2]
  /home/charlesbluca/Documents/GitHub/compose/etc/conda/cuda_11.2/envs/rapids/lib/python3.8/site-packages/pandas/core/dtypes/missing.py:484: DeprecationWarning: elementwise comparison failed; this will raise an error in the future.
    if np.any(np.asarray(left_value != right_value)):

-- Docs: https://docs.pytest.org/en/stable/warnings.html

Is there something happening in the as_column operation here that would be causing the elementwise comparison to fail?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert_eq does it's final checking in a loop in python/pandas land. It's probably trying to compare two non-scalar objects ultimately and something weird is happening. I'd suggest calling to_pandas on the cuDF side of the comparison and seeing what the things that are being compared look like in the end.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking out to_pandas shows the two objects do look the same:

>>> cudf.Series(actual_column).to_pandas()
0       [[1, 2, 3], [4, 5, 6]]
1    [[7, 8, 9], [10, 11, 12]]
dtype: object
>>> cudf.Series(expected).to_pandas()
0       [[1, 2, 3], [4, 5, 6]]
1    [[7, 8, 9], [10, 11, 12]]
dtype: object

Could this warning just be a consequence of comparing list columns with nested lists?

Copy link
Contributor

@isVoid isVoid Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. When cudf objects convert to pandas, it first goes through pyarrow. Upon converting arrow to pandas format, it converts nested lists into nested arrays:

>>> arr = pa.array([[1, 2, 3]])
>>> arr
<pyarrow.lib.ListArray object at 0x7f43187e8a60>
[
  [
    1,
    2,
    3
  ]
]
>>> arr.to_pandas()
0    [1, 2, 3]
dtype: object
>>> arr.to_pandas().iloc[0]
array([1, 2, 3])

),
],
)
def test_as_column_arrow_array(data, expected):
actual_column = cudf.core.column.as_column(data)
assert_eq(cudf.Series(actual_column), cudf.Series(expected))


@pytest.mark.parametrize(
"pd_dtype,expect_dtype",
[
Expand Down