cudf-polars string/numeric casting

[brandon-b-miller]

Depends on #16991
Part of #17060

Implements cross casting from string <-> numeric types in cudf-polars

[Matt711]

I left some questions. And I think the PR needs the latests changes from branch-24.12.

[brandon-b-miller]

@wence- in this case polars is testing for a specific exception when a string can't be cast to an integer. We're able to raise an exception in the right place on the cudf-polars side, but because of the way we wrap it in a polars.exceptions.ComputeError we assert False in the end.

FAILED py-polars/tests/unit/sql/test_cast.py::test_cast_errors[values5-values::int4-conversion from `str` to `i32` failed] - polars.exceptions.ComputeError: InvalidOperationError: Conversion from `str` failed.

The easiest thing would be to add this to the list of expected failures. That said, have we ever explored the idea of propagating the original exception back to the user in the case of runtime errors?

[wence-]

@wence- in this case polars is testing for a specific exception when a string can't be cast to an integer. We're able to raise an exception in the right place on the cudf-polars side, but because of the way we wrap it in a polars.exceptions.ComputeError we assert False in the end.

FAILED py-polars/tests/unit/sql/test_cast.py::test_cast_errors[values5-values::int4-conversion from `str` to `i32` failed] - polars.exceptions.ComputeError: InvalidOperationError: Conversion from `str` failed.

The easiest thing would be to add this to the list of expected failures. That said, have we ever explored the idea of propagating the original exception back to the user in the case of runtime errors?

I've thought about it, but I think it's fiddly because this execution happens inside polars rust, which doesn't know about python exception types, I think.

Can you open an issue/feature in polars to discuss this please?

[wence-]

Surely this will go on one line if we remove the final trailing comma.

[wence-]

For floating point types, can we please test:

• negative numbers
• +/- inf
• nan
• scientific notation

In addition.

[brandon-b-miller]

Argh, libcudf gives us Inf and polars gives us inf. :(

[wence-]

Does polars ingest Inf fine?

[wence-]

If so, just compare case insensitively

[wence-]

This looks good to me, thanks. Can you just check if there are other places where we could now be using astype?

[mhaseeb123]

C++ changes look good!

[vyasr]

Any idea why tests are failing here?

[Matt711]

I think (from this comment) you still intend to add this as an expected failure here?

[wence-]

I'm not sure about this one, the range of int64 is contained in the range of float32. I believe the mapping preserves the order.

Also, what about float to integral? I suppose it depends on what happens to the out of bounds values.

[brandon-b-miller]

Correct the float to integral cases all fall into the last return False because the out of range values might lose their ordering in the cast.

[wence-]

Why? If the range is just clamped, then you have no problem, ordering is preserved.

[brandon-b-miller]

Because I expect the equivalent of this:

>>> ary = np.array([1, 2, float(np.iinfo('int64').max) + 1])
>>> ary
array([1.00000000e+00, 2.00000000e+00, 9.22337204e+18] # ordered
>>> ary.astype('int64')
array([1, 2, -9223372036854775808]) # not ordered

[wence-]

Is that what cudf does though?

[wence-]

With wrap_numerical=True in the cast for polars, it clamps, AFAICT

[brandon-b-miller]

Is that what cudf does though?

no, libcudf clamps. If we're ok encoding libcudf specific implementation behavior into this function we could pass the float to int cases.

[brandon-b-miller]

@wence- is this current behavior considered buggy? It's almost like we should be raising unless wrap_numeric==True.

>>> df.collect()
shape: (3, 1)
┌───────────┐
│ a         │
│ ---       │
│ f32       │
╞═══════════╡
│ 1.0       │
│ 2.0       │
│ 9.2234e18 │
└───────────┘
>>> df.select(pl.col('a').cast(pl.Int64)).collect()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/envs/cudf_dev/lib/python3.12/site-packages/polars/lazyframe/frame.py", line 2055, in collect
    return wrap_df(ldf.collect(callback))
                   ^^^^^^^^^^^^^^^^^^^^^
polars.exceptions.InvalidOperationError: conversion from `f32` to `i64` failed in column 'a' for 1 out of 3 values: [9.2234e18]
>>> df.select(pl.col('a').cast(pl.Int64)).collect(engine=pl.GPUEngine())
shape: (3, 1)
┌─────────────────────┐
│ a                   │
│ ---                 │
│ i64                 │
╞═════════════════════╡
│ 1                   │
│ 2                   │
│ 9223372036854775807 │
└─────────────────────┘

[wence-]

Probably yes, we should barf for the cases where we're strictish mode because we haven't implemented those

[brandon-b-miller]

The issue I forsee here is that wrap_numerical=False, strict=True is the default. This means that by default the GPU backend will also have to scan during the float-int cast for the presence of these values and throw. This is shaping up to be a pattern that occurs in several places within the codebase, and it's probably not ideal to need to scan before every cast.

For now I have passed the float-int conversions through this function which retains the existing behavior, since regardless of if OOB values are nullified or clamped we'll retain order. I will raise a separate issue to discuss the proliferation of scanning as a result of polars defaults.

[wence-]

Thanks Brandon, happy with the current state.

Can you please write up a bit more detail in #17244 about the different casting modes, what cudf-polars currently does, and the routes to supporting them.

[vyasr]

IIUC this PR is ready to merge and @brandon-b-miller just needs to add a bit more info to #17244, right?

[brandon-b-miller]

/merge