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

[REVIEW] Fix data corruption in string columns #7746

Merged
merged 4 commits into from
Mar 29, 2021
Merged

[REVIEW] Fix data corruption in string columns #7746

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66a5283
fix base_size calculation of string column
galipremsagar Mar 28, 2021
c6d7321
add distributed round trip test
galipremsagar Mar 28, 2021
78d6dc0
add test
galipremsagar Mar 28, 2021
7f279d1
add serialization tests
galipremsagar Mar 28, 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
13 changes: 5 additions & 8 deletions python/cudf/cudf/core/column/string.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@
is_space as cpp_isspace,
is_upper as cpp_is_upper,
)
from cudf._lib.strings.convert.convert_integers import (
is_integer as cpp_is_integer,
)
from cudf._lib.strings.convert.convert_floats import is_float as cpp_is_float
from cudf._lib.strings.combine import (
concatenate as cpp_concatenate,
join as cpp_join,
Expand All @@ -91,6 +87,10 @@
from cudf._lib.strings.convert.convert_fixed_point import (
to_decimal as cpp_to_decimal,
)
from cudf._lib.strings.convert.convert_floats import is_float as cpp_is_float
from cudf._lib.strings.convert.convert_integers import (
is_integer as cpp_is_integer,
)
from cudf._lib.strings.convert.convert_urls import (
url_decode as cpp_url_decode,
url_encode as cpp_url_encode,
Expand Down Expand Up @@ -4760,10 +4760,7 @@ def base_size(self) -> int:
if len(self.base_children) == 0:
return 0
else:
return int(
(self.base_children[0].size - 1)
/ self.base_children[0].dtype.itemsize
)
return self.base_children[0].size - 1

@property
def data_array_view(self) -> cuda.devicearray.DeviceNDArray:
Expand Down
14 changes: 14 additions & 0 deletions python/cudf/cudf/tests/test_string.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2922,3 +2922,17 @@ def test_string_std():
assert_exceptions_equal(
lfunc=psr.std, rfunc=sr.std, compare_error_message=False
)


def test_string_slice_with_mask():
actual = cudf.Series(["hi", "hello", None])
expected = actual[0:3]

assert actual._column.base_size == 3
assert_eq(actual._column.base_size, expected._column.base_size)
assert_eq(actual._column.null_count, expected._column.null_count)
assert_eq(
actual._column.mask.to_host_array(),
expected._column.mask.to_host_array(),
)
assert_eq(actual, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we convert this to something that tests user-facing behaviour rather than internal behaviour?

In other words, did this bug manifest in a way that affected end-users? If so, can we test that we fixed that instead?

Copy link
Contributor Author

@galipremsagar galipremsagar Mar 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I had this similar thought initially and thought we could check with isnull public API, but since this goes to libcudf call and that returns the correct result without interacting with Column.mask we cannot validate using Series.isnull.

The closest user-facing behavior where this issue would surface is when we round-trip(when it goes through serialize) a series with string dtype & having nulls like in test_distributed::test_str_series_roundtrip.

Had to add both user-facing & internal test because we don't seem to validate base_size anywhere except for this test where we only test against an empty column:

cudf/python/cudf/cudf/tests/test_string.py

Lines 1315 to 1318 in ccc28d5

def test_string_no_children_properties():
empty_col = StringColumn(children=())
assert empty_col.base_children == ()
assert empty_col.base_size == 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we should definitely add a test for that then (maybe in test_serialize.py?).

We can keep this test in addition, if you prefer. Personally, I'm not a fan of testing internals, but that could be just me :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a serialization test where we would still have to validate an internal component(i.e., the frames) and removed checking for mask and retained base_size checks in test_string.py

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand -- what internal attribute are we testing in the new serialize test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Serialize returns a dict & Frames as buffers. The internal attribute we are testing here is Frames([index_frame, offset_frame, chars_frame, mask_frame]), to be specific we want to validate the mask_frame at last index which would be the right validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks!

14 changes: 12 additions & 2 deletions python/dask_cudf/dask_cudf/tests/test_distributed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
from dask.distributed import Client
from distributed.utils_test import loop # noqa: F401

import dask_cudf

import cudf
from cudf.tests.utils import assert_eq

import dask_cudf

dask_cuda = pytest.importorskip("dask_cuda")


Expand Down Expand Up @@ -65,3 +65,13 @@ def test_ucx_seriesgroupby():
dask_df_g = dask_df.groupby(["a"]).b.sum().compute()

assert dask_df_g.name == "b"


def test_str_series_roundtrip():
with dask_cuda.LocalCUDACluster(n_workers=1) as cluster:
with Client(cluster):
expected = cudf.Series(["hi", "hello", None])
dask_series = dask_cudf.from_cudf(expected, npartitions=2)

actual = dask_series.compute()
assert_eq(actual, expected)