Cory Grinstead
05/02/2025, 1:19 AMCory Grinstead
05/02/2025, 1:21 AMfloat 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.Cory Grinstead
05/02/2025, 1:24 AMCory Grinstead
05/07/2025, 12:14 AMCory Grinstead
05/07/2025, 12:20 AMKevin Wang
05/07/2025, 12:20 AMCory Grinstead
05/07/2025, 12:30 AMCory Grinstead
05/07/2025, 7:17 PMCory Grinstead
05/07/2025, 8:08 PMKevin Wang
05/07/2025, 9:53 PMto_field and evaluate implementations to handle that themselves. Is that correct?Kevin Wang
05/07/2025, 10:59 PMto_field and evaluate could then just take in that struct as an input.
Here's an example of what I'm thinking:
// 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> {
...
}
}Cory Grinstead
05/07/2025, 11:40 PMKevin Wang
05/07/2025, 11:44 PMNamedExpr . 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?Kevin Wang
05/07/2025, 11:47 PMExpr::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 sometimeCory Grinstead
05/07/2025, 11:51 PMNamedExpr . 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.Kevin Wang
05/07/2025, 11:54 PMCory Grinstead
05/08/2025, 12:04 AMKevin Wang
05/08/2025, 12:07 AMCory Grinstead
05/08/2025, 1:55 AMnamed_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 partyKevin Wang
05/08/2025, 1:57 AMCory Grinstead
05/08/2025, 2:35 AMtrait ScalarUDF {
type Args;Cory Grinstead
05/08/2025, 2:38 AMevaluate/to_field . We could add something similar later on if it becomes necessary.Kevin Wang
05/08/2025, 3:15 AMKevin Wang
05/08/2025, 3:18 AM