Removing int8 column option from parquet byte_array writing

[hyperbolic2346]

Description

As suggested in #11526 and captured in issue #11536 the usage of both INT8 and UINT8 as supported types for byte_arrays is unnecessary and adds complexity to the code. This change removes INT8 as an option and only allows UINT8 columns to be written out as byte_arrays. This matches with cudf string columns which contain an INT8 column for data.

closes #11536

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[codecov]

Codecov Report

Base: 87.40% // Head: 88.37% // Increases project coverage by +0.96% 🎉

Coverage data is based on head (86942b2) compared to base (f72c4ce).
Patch coverage: 86.52% of modified lines in pull request are covered.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #11539      +/-   ##
================================================
+ Coverage         87.40%   88.37%   +0.96%     
================================================
  Files               133      133              
  Lines             21833    22508     +675     
================================================
+ Hits              19084    19892     +808     
+ Misses             2749     2616     -133     

Impacted Files	Coverage Δ
python/cudf/cudf/core/udf/__init__.py	97.05% <ø> (+47.05%)	⬆️
python/cudf/cudf/io/orc.py	92.94% <ø> (-0.09%)	⬇️
...thon/dask_cudf/dask_cudf/tests/test_distributed.py	18.86% <ø> (+4.94%)	⬆️
python/cudf/cudf/core/_base_index.py	82.20% <43.75%> (-3.35%)	⬇️
python/cudf/cudf/io/text.py	91.66% <66.66%> (-8.34%)	⬇️
python/strings_udf/strings_udf/__init__.py	86.27% <76.00%> (-10.61%)	⬇️
python/cudf/cudf/core/index.py	93.18% <95.16%> (+0.55%)	⬆️
python/cudf/cudf/__init__.py	90.69% <100.00%> (ø)
python/cudf/cudf/core/column/categorical.py	89.34% <100.00%> (ø)
python/cudf/cudf/core/groupby/groupby.py	91.51% <100.00%> (+0.29%)	⬆️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[upsj]

LGTM except for the aforementioned concerns about values outside int8's range.

[davidwendt]

Based on the description, I would've expected this would strictly used UINT8 instead of INT8 since the sign bit has no meaning in plain binary data like this. Also, it would probably help with the boundary issues mentioned in the other review comments as well.
I think the consistency with strings argument is overly emphasized and should really not be a consideration in this implementation.

[hyperbolic2346]

I think the consistency with strings argument is overly emphasized and should really not be a consideration in this implementation.

I would typically use unsigned int values for bytes as well and I would agree with you about the string argument if it weren't for the strong coupling between the two that exists. When reading binary, we read it as a string always and can decompose the column and built it again as a list of bytes if desired. The column type that we get from pulling apart a string column is an int8 column, for reasons unknown to me. This seemed to imply a precedence for int8 byte representations inside cudf and also made the easy path for conversion to be list. As a result, reading binary data from a parquet file returns a list type column right now.

That said, the easy path doesn't mean that it is the best path by a long shot. If we have consensus that uint8 should be the type we use and expect, I am ok with that, but it will require more than a few changes. It is certainly more natural to fit into the parquet statistics.

[etseidl]

The column type that we get from pulling apart a string column is an int8 column, for reasons unknown to me.

Beyond scope for this PR, but I'd be interested in a discussion about this. Since strings in cuDF are UTF-8, not c-style char arrays, why not use UINT8 as the child column type for both binary and string data? Is it for ease of binding to python strings? string_view has to cast to unsigned char * to compare two strings already. Then again, casts are cheap and refactoring expenssive, so 🤷

[hyperbolic2346]

why not use UINT8 as the child column type for both binary and string data

From what I can gather, when strings were implemented cudf didn't have a uint8 type. I agree that unsigned is a better option, but I wonder the impact that would have externally by users. I think the best options is to create an issue to discuss changing the behavior.

[davidwendt]

I don't think consistency with a different type (i.e. string_view) is a compelling reason to use INT8 here. The string_view has its own requirements and dependencies (and things that depend on it). The design and implementation of string_view can be discussed outside the scope of this PR. Regardless, I see no reason to prevent a byte-array to use UINT8.

[hyperbolic2346]

Updated this to change the column type to uint8 instead of int8. Now binary writes only accept uint8 columns and binary reads come back as uint8 columns.

[rwlee]

Also resolves NVIDIA/spark-rapids#6408 with the added changes that standardize casting to list of bytes. There should be no additional changes needed on the plugin side based on my testing + integration tests.

[bdice]

Approving with one question. Happy to defer on that if you don't want to make those changes in this PR.

[bdice]

Should we fetch the null count before releasing the data, and pass it here? We should try to retain any information that may have been previously computed.

Generally, I think we're going to move away from lazily computing null counts as we work on adding streams to the public API. There are some hiccups with the lazy approach and streams, but primarily we aren't being as efficient as possible wrt avoiding calls to the null_count kernel because we aren't tracking information that we already may have computed once earlier.

[rwlee]

I like the idea of moving away form lazily computed null counts and am happy to change here if we want to start now. In this case, it made sense for me to defer the null counting because byte cast is often use as an intermediate type for reading, writing, and hashing. Those are usually indifferent to null count and it made sense to keep it lazily computed while we still supported it.

[rwlee]

Adding do not merge because of some upmerge issues. Upmerged to branch-22.12 in order to resolve some python errors and tested locally successfully... Continuing to debug but i don't want this merged accidently.

[bdice]

rerun tests

[bdice]

@gpucibot merge