[REVIEW] Fix data corruption in string columns

[galipremsagar]

Fixes: #7735

Minimal repro of the above issue is:

>>> import cudf
>>> s = cudf.Series(['hi', 'hello', None])
>>> s
0       hi
1    hello
2     <NA>
dtype: string
>>> h = s[0:3]
0       hi
1    hello
2     <NA>
dtype: string
>>> s._column.null_count
1
>>> h._column.null_count
1

Incorrect mask calculation in Column.from_column_view because of incorrect base_size calculation in StringColumn:

>>> s._column.mask.to_host_array()
array([3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
       0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
       0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
      dtype=uint8)
>>> h._column.mask.to_host_array()
array([], dtype=uint8) # Should have a mask similar to above one.

>>> s._column.base_size
0 # Should be 3
>>> h._column.base_size
0 # Should be 3

So in this PR I have fixed the calculation of StringColumn.base_size and introduced tests to have a check for the same.

[shwina]

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?

[galipremsagar]

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

[shwina]

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 :-)

[galipremsagar]

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

[shwina]

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

[galipremsagar]

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.

[shwina]

Got it. Thanks!

[codecov]

Codecov Report

Merging #7746 (7f279d1) into branch-0.19 (ccc28d5) will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           branch-0.19    #7746      +/-   ##
===============================================
+ Coverage        82.13%   82.54%   +0.40%     
===============================================
  Files              101      101              
  Lines            17096    17461     +365     
===============================================
+ Hits             14042    14413     +371     
+ Misses            3054     3048       -6     

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/string.py	86.79% <100.00%> (+0.21%)	⬆️
python/cudf/cudf/io/feather.py	100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py	0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py	100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py	0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py	100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py	100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py	100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py	100.00% <0.00%> (ø)
... and 38 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccc28d5...7f279d1. Read the comment docs.

[kkraus14]

@gpucibot merge