[REVIEW] Adding support for unsigned int

[rgsl888prabhu]

Adds support of unsigned int to cudf, updated validations and test cases to accommodate it.

And instead of -1 as sentinel value, max(int64) is being used in most of the cases.

[cwharris]

🙌

[kkraus14]

I'm super uncomfortable with using 0 as a sentinel for nulls throughout the tests. It seems very error prone and fragile. Any ideas on what we can do instead? If we wait for the Pandas 1.0 PR do the new Pandas nullable types help?

[rgsl888prabhu]

Will this close #2819?

It will, but there are other issues such as #5352 and #5351 which still requires changes in libcudf side.

[rgsl888prabhu]

I'm super uncomfortable with using 0 as a sentinel for nulls throughout the tests. It seems very error prone and fragile. Any ideas on what we can do instead? If we wait for the Pandas 1.0 PR do the new Pandas nullable types help?

It might help, but that would require lot of additional changes to testing to accommodate that change and how close are we with supporting pandas 1.0?

[shwina]

I'm super uncomfortable with using 0 as a sentinel for nulls throughout the tests. It seems very error prone and fragile. Any ideas on what we can do instead?

I agree 100% here. Here are some additional considerations:

1. 0 should be considered as good a sentinel value as any other, i.e., just changing our sentinel value to something else doesn't guarantee our tests aren't broken.

2. Regarding Pandas 1.0, do they support nulls for all the operations that we have? If so, do their null semantics match ours? If not, this will be difficult to incorporate in the short term.

I think that our feature set is at a point where comparing with Pandas isn't sufficient and/or possible in many cases. For these cases, we should determine what we think is the right behaviour and test against that, rather than what Pandas does for some different (but closely related) case.

That being said, I'm not advocating for not testing against Pandas. Where possible, we should definitely still do that.

[rgsl888prabhu]

@kkraus14 @shwina As a temporary solution till we get Pandas 1.0 support, shall we use max of the col dtype to signify null. At least this value will not be as common as 0.

[kkraus14]

@kkraus14 @shwina As a temporary solution till we get Pandas 1.0 support, shall we use max of the col dtype to signify null. At least this value will not be as common as 0.

It would be slightly better, but it still feels extremely fragile

[shwina]

@kkraus14 @shwina As a temporary solution till we get Pandas 1.0 support, shall we use max of the col dtype to signify null. At least this value will not be as common as 0.

Practically speaking, yes, I think this is better than 0, but theoretically, max(int64) isn't any better than 0. Neither is 17 or anything else for that matter :)

Suppose we decided to re-write the tests to use Pandas nullable types where possible, and our own test data otherwise, what's the level of effort involved? How many tests would this touch? Is this something that a couple of us could pool effort on and pull off?

[rgsl888prabhu]

Just to test, I just changed sentinel value to something random, and I can observe around 10 - 15 tests failing, there can be more than that. But it feels like doable. As @brandon-b-miller mentioned in #5388 , we might have to introduce dtypes to handle nulls, which might add additional burden on modifying these test cases.

[kkraus14]

Lets use max(uint64) as the sentinel value for now (where we can) for the tests, and we'll file issues and follow up in using Pandas nullable types / rolling our own null behaviors in tests as needed to move away from the sentinel value based testing.

[rgsl888prabhu]

Changed the sentinel value to be max(int64) rather than max(uint64) as the value of an int can't be greater than sys.maxsize, else any conversion will fail with following error.

>>> np.int8(np.iinfo(np.uint64).max)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C long

I have used this sentinel value in most of the places apart from places were float column is being created and then it is casted to integer.

[kkraus14]

@shwina can you review again before we merge?

[kkraus14]

@shwina can you review again before we merge?

connected offline and confirmed this is good to go so merging!