jayzhan211 commented on issue #11359:
URL: https://github.com/apache/datafusion/issues/11359#issuecomment-2220320006
@ozankabak had asked for whether there is anyway to entirely getting rid of
logical expressions in discord, so I think we can review about the challenge I
had before.
The reason why there are logical expressions in `creeate_aggregate_expr` is
for customizing `Accumulator`. We can have different kind of accumulator based
on the function's arguments. Ideally we should check with PhysicalExpr, but
unfortunately, given the current design, we are not able to introduce physical
expressions in `AggregateUDFImpl` trait, because we want to avoid adding
dependency of `physical-expr` crate to `datafusion-expr`.
I think that it is also the main reason that blocking us. I propose to
redesign about the role of each crate. To able to deal with physical concept
for `AggregateUDFImpl`, we break `physical-expr` into two level. One is
`physical-expr-common`, and another is `physical-expr`. The same applies to
logical expr, `datafusion-expr-common` and `datafusion-expr`. `common crate`
is the higher level crate so that we can **add the dependency of
`physical-expr-common` into `datafusion-expr`**
The crate graph is like
```mermaid
graph TD;
functions-aggregates-common-->expr-common;
functions-aggregates-common-->physical-expr-common;
functions-aggregates-->functions-aggregates-common;
functions-aggregates-->expr;
physical-expr-common-->expr-common;
expr-->expr-common;
expr-->functions-aggregates-common;
physical-expr-->physical-expr-common;
core-->functions-aggregates;
core-->physical-expr;
third-parties-aggregate --> functions-aggregates-common;
```
`expr-common`: Things other than `Expr` and `LogicalPlan` can place it here
`expr`: Mainly for `Expr` and `LogicalPlan`. Import
`functions-aggregate-common` for UDAF.
`physical-common`: Physical expr trait or other common things. Similar to
what it is now **except physical-expr like Column, Cast, Literal which should
move to physical-expr**
`physical-expr`: Physical Expr are here + Column, Cast, Literal
`functions-aggregate-common`: Import `physical-common` and `expr-common`,
for other users to build their own udaf.
`functions-aggregate`: datafusion builtin functions
The more detail of discussion before in
https://github.com/apache/datafusion/issues/10074
With this approach, function like
`limited_convert_logical_expr_to_physical_expr` is no longer needed after this
change.
@alamb I think we can review about this idea again, the previous concern is
that
> What I am worried about is that physical-expr-common would end up with all
the code from physical-expr
I think it is not an issue anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
