part 3 of my expr refactor PR's is now done. This ...
# daft-dev
c
part 3 of my expr refactor PR's is now done. This one looks pretty big, but there's not a ton of actual logic in it. happy to walk through it in more detail tomorrow if there are questions. https://github.com/Eventual-Inc/Daft/pull/4286. I'd like to get all 3 of these merged by EOW if possible. CC @Kevin Wang @R. C. Howell @Sammy Sidhu
❤️ 2
also note: these 3 pr's only are refactoring the
float
and
numeric
namespaces/modules over to the new pattern. If this all looks good, then I'm happy to continue getting everything else moved over. I wanted to get atleast a couple moved over end-to-end to make sure the refactor went as planned before doing everything.
also incidentally, this PR makes it so functions enforce positional before named as well! @R. C. Howell I know you've been wanting that for a while now!
just a reminder, this PR is still waiting on review. cc @Sammy Sidhu @Kevin Wang @R. C. Howell. I have a few followup PR's that depend on this one as well. • https://github.com/Eventual-Inc/Daft/pull/4302 • https://github.com/Eventual-Inc/Daft/pull/4312 (not yet finished, but will be done early tomorrow)
I know it's kind of bigger PR. (i tried hard to keep it small, but it got away from me 😅 ). So i'd be happy to do a synchronous review on it if that'd be helpful. FWIW, the smaller subsequent PR's give a good glimpse into the intended usage. Especially the json one! https://github.com/Eventual-Inc/Daft/pull/4302
k
Will take a look tomorrow!
c
I also added some notes to the PR for specific areas to review.
Just finished up 4312, and we were able to remove a lot of boilerplate code
In an attempt to make it a bit more digestible, here's a cherrypicked PR that only has the most relevant changes. https://github.com/Eventual-Inc/Daft/pull/4323/files
k
@Cory Grinstead so it looks to me that ScalarUDF itself does not have any sort of function argument validation, and we are deferring to the
to_field
and
evaluate
implementations to handle that themselves. Is that correct?
This is maybe a wild idea but have you considered having some sort of function arguments trait that can be implemented via a macro? the macro could create a struct for the arguments of a specific function and handle implementing a validator that constructs the struct from the inputs.
to_field
and
evaluate
could then just take in that struct as an input. Here's an example of what I'm thinking:
Copy code
// defines required arguments "a" and "b", variadic argument "rest", and optional argument "c" with default value of lit(0)
create_args_struct!(FooArgs, a, b, *rest, c = 0)

// the generated struct would look like:
struct FooArgs<T> {
    a: T,
    b: T,
    rest: Vec<T>,
    c: T
}

// this will also be automatically implemented for FooArgs
impl<T> TryFrom<FunctionArgs<T>> for FooArgs<T> {
    fn try_from(args: FunctionArgs<T>) -> DaftResult<Self<T> {
        ...
    }
}

// then, you would use it like:
impl ScalarUDF for Foo {
    type Args<T> = FooArgs<T>;

    fn to_field(&self, inputs: FooArgs<ExprRef>) -> DaftResult<Field> {
        ...
    }
    
    fn evaluate(&self, inputs: FooArgs<Series>) -> DaftResult<Series> {
        ...
    }
}
c
🤔 that's an interesting idea. I'll play around with it and see if i can get anything going
k
Cool. Everything else looks good. The only other comment I had about structure was on
NamedExpr
. My understanding is that we need that expression variant right now, because if ScalarFunction stored
FunctionArgs<ExprRef>
instead, it would be difficult to implement a proper
Expr::children
and
Expr::with_new_children
, right?
We actually have a similar problem with window functions too where
Expr::children
currently does not list the partition by and order by keys, since it does not give sufficient information to construct the window function again using
Expr::with_new_children
. I checked DataFusion and they actually updated their treenode stuff after we forked it to deal with stuff like this. We should merge in the upstream changes sometime
c
> The only other comment I had about structure was on
NamedExpr
. My understanding is that we need that expression variant right now, because if ScalarFunction stored
FunctionArgs<ExprRef>
instead, it would be difficult to implement a proper
Expr::children
and
Expr::with_new_children
, right? not exactly. Similar to SQL and python, we want a way for
ScalarUDF
to handle either named or positional arguments, or any combination there of. In many functions we only want to use kwargs, So we need some way to associate a name to an expression for this purpose only. I'm sure there's an alternative approach that may be more elegant, but it seemed simple enough to attach a name to the expr so when we call
evaluate
or
to_field
we have that additional context of if it was an
arg
or
kwarg
.
k
Wouldn't directly storing the arguments as FunctionArgs solve that problem? It gives both positional as well as named information.
c
🤔 hmm yeah I guess so. Let me see if i can get that to work.
k
Would be nice not to have the variant if not needed. It's not a big problem but it does feel like abusing the Expr enum a bit for representing info specific to a scalar function, and has the potential to cause weird bugs in the future, if, say, we change the expression during simplification.
c
@Kevin Wang I did manage to remove the
named_expr
variant here https://github.com/Eventual-Inc/Daft/pull/4286/commits/a6d099a4db36dde75d4880a92261a1e5a85e66bc. It added some boilerplate to the sql crate, but most of that'll be going away as we move to the centralized functions_registry anyway. thanks for the suggestion daft party
k
Great! And how is my arg macro idea shaping up if you're still exploring that?
c
i gave up on the macro stuff. You can't do associated types with typetag. So this can't work
Copy code
trait ScalarUDF {
  type Args;
we did something similar in the sql layer, but without macros. https://github.com/Eventual-Inc/Daft/blob/a74dd02f15126c7dcb76b488b4ab21f508bd2b7e/src/daft-sql/src/functions.rs#L542. But I also think it's easy enough handling the arguments directly inside
evaluate/to_field
. We could add something similar later on if it becomes necessary.
k
Ah makes sense. I mostly just wish we had a way to define the accepted arguments in one place for each function. it would reduce validation boilerplate and also allow for maybe autogeneration of function docs for SQL and other APIs. but we don’t need to include that in the scope of this PR
Will put a review on Github tomorrow!