Migrate expressions to pylibcudf

[lithomas1]

Description

xref #15162

Migrates expresions to use pylibcudf.

Checklist

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

[vyasr]

I think we should take a slightly different route here than we currently are with Literals. Instead of having the objects own both a libcudf scalar and a libcudf expression, I think we should have them own a pylibcudf Scalar and a libcudf expression. Right now the only way to construct pylibcudf Scalars is via arrow interop, but the logic that we're using in this file to generate different scalar types is essentially the same as what we would want in a Scalar constructor. Furthermore, once we do that we could replace the branching logic in this PR using a libcudf type_dispatcher-based approach that should make it cleaner and easier to maintain. This separation of concerns would also allow us to transparently support arrow scalars, like you asked. I realize that's a pretty big change from how this PR is currently structured, though, and I'm happy to work with you on that if you'd like!

[lithomas1]

Draft pending resolution of Vyas's comments.

[lithomas1]

@vyasr Let's put this in as is for now?

[lithomas1]

There was a use-after-free here (in the old cudf/_lib/expressions.pyx) previously, but it didn't get caught I think because the inputs to Operations weren't gc'ed yet.

[lithomas1]

In general, if the C++ side takes a reference, we need to hold on to the object on the Python side, right?

[vyasr]

Yup we do, that's a good catch.

[wence-]

The previous approach was "person constructing the expression must keep everything alive".

[vyasr]

Let's merge and refactor later.

[vyasr]

Yup we do, that's a good catch.

[wence-]

Ideally, we would cull the numpy requirement and use pylibcudf scalars.

[wence-]

The previous approach was "person constructing the expression must keep everything alive".

[wence-]

I would love to drop the runtime numpy dependency. What would it take to use pylibcudf datatypes instead?

[wence-]

I think this should accept a value and a pylibcudf DataType then we don't have to do (dangerous) introspection of the value to determine what scalar we will make.

Alternatively, we could accept a pylibcudf Scalar, and then (?) borrow the reference for our c_scalar?

Either of those changes would also obviate the need to have a runtime dependency on numpy.

In both cases we need to have a dispatch/type_mapping from datatype to concrete scalar type but it's more explicit than introspection.

Having thought a bit more, we should use a pylibcudf Scalar I think, and then do something like:

def __cinit__(self, Scalar value):
    self.scalar = value
    cdef data_type typ = value.type()
    cdef type_id tid = typ.id()
    if not (plc.traits.is_numeric(data_type) or plc.traits.is_chrono(data_type) or tid == STRING):
        raise ...
    if tid == plc.TypeID.INT8:
         self.c_obj = <unique_ptr[scalar]>move(make_unique[literal](<numeric_scalar[int8_t] &>dereference(value.c_obj)))
    elif tid == plc.TypeId.INT16:
        ...

[wence-]

It would be nice if we could pass a type-erased scalar to the ast::literal constructor, but my type-dispatching-fu is not sufficient.

[wence-]

Ah, I realise I had the same high-level issue as @vyasr, it seems like you'd decided to do that separately?

[lithomas1]

Ah, I realise I had the same high-level issue as @vyasr, it seems like you'd decided to do that separately?

Yeah, I kinda thought about this for a bit, but it's a pretty involved fix.
(but eventually, I would like this to take a pylibcudf Scalar)

Easy way out is:
Just convert everything to a pyarrow scalar and call interop on it (this is what DeviceScalar does). This is probably not good for the goal of getting pyarrow out of the Cython.

I could also try to delete the datetime/timedelta handling code in this PR.
(I don't think that gets used in cuDF Python)

The more correct way is to use the interchange mechanisms on the array object/scalar object we take in which I talked about with Vyas.

IMO, the best way to do this is to use DLPack, so we only have to support one thing.
(Not sure if we need C++ changes here, though )

[wence-]

(but eventually, I would like this to take a pylibcudf Scalar)

Why is this one so hard? I think you just need to have a big switch statement in the constructor that takes the type-erased Scalar object and constructs (via casting) the correct numeric_scalar/timestamp_scalar/duration_scalar.

Effectively you would just replace the current switch statement that dispatches on numpy-like types with one that dispatches on scalar.type().id(), I think.

What am I missing?

[wence-]

I could also try to delete the datetime/timedelta handling code in this PR. (I don't think that gets used in cuDF Python)

This is kind of critical for predicate pushdown in cudf-polars.

[lithomas1]

(but eventually, I would like this to take a pylibcudf Scalar)

Why is this one so hard? I think you just need to have a big switch statement in the constructor that takes the type-erased Scalar object and constructs (via casting) the correct numeric_scalar/timestamp_scalar/duration_scalar.

I meant creating the Scalar itself.
The Scalar constructor has to accept a lot of possible inputs (e.g. Python scalars, Numpy scalars, Pyarrow scalars, potentially cupy scalars)

I think your proposed approach would work after we fixed the Scalar constructor.
(actually now that I think about it, maybe fixing this isn't a very immediate concern, I thought we'd need pyarrow in the Cython, but we can just force people to do plc.interop.from_arrow in their Python code outside the Cython)

EDIT: I will try the approach taking in a Scalar.

[wence-]

I think your proposed approach would work after we fixed the Scalar constructor.

Yeah, I think from the pylibcudf point of view in some sense, for the expression API, it is not (or should not be) the job of pylibcudf to help with the construction of a Scalar in the first place.

[wence-]

Thanks! Trivial grammar.

[lithomas1]

/merge