ExprBuilder for Physical Aggregate Expr #11617
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
jayzhan211
force-pushed
the
physical-expr-builder
branch
from
953aceb to
315b8e9
Compare
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
| })) | ||
| } | ||
|
|
||
| pub fn name(mut self, name: impl Into<String>) -> Self { |
There was a problem hiding this comment.
I'm not if it is necessary, maybe we can build it from function+args, so keep it out from new() so it is easier for deprecation
There was a problem hiding this comment.
I agree creating the name automatically from existing information would be better. Perhaps we can do that as a follow on PR
| self | ||
| } | ||
|
|
||
| pub fn schema(mut self, schema: SchemaRef) -> Self { |
There was a problem hiding this comment.
Not sure whether we need schema or dfschema after refactor, so keep them out from new to prepare for possible deprecation
| false, | ||
| ) | ||
| .unwrap() | ||
| AggregateExprBuilder::new(count_udaf(), vec![self.column()]) |
There was a problem hiding this comment.
I think that is much easier to understand 💯
| })) | ||
| } | ||
|
|
||
| pub fn name(mut self, name: impl Into<String>) -> Self { |
There was a problem hiding this comment.
I agree creating the name automatically from existing information would be better. Perhaps we can do that as a follow on PR
| None::<String>, | ||
| field_name, | ||
| ))), | ||
| expr: Box::new(col(field_name)), |
datafusion/proto/Cargo.toml
Outdated
| @@ -50,6 +50,7 @@ chrono = { workspace = true } | |||
| datafusion = { workspace = true, default-features = true } | |||
| datafusion-common = { workspace = true, default-features = true } | |||
| datafusion-expr = { workspace = true } | |||
| datafusion-physical-expr-common = { workspace = true } | |||
There was a problem hiding this comment.
since proto already relies on the core datafusion crate this is not a new (direct) dependency.
Though maybe it highlights that it would be good to pub use the AggregateExprBuilder in datafusion so using it doesn't need a new explicit dependency 🤔
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct AggregateExprBuilder { |
There was a problem hiding this comment.
| pub struct AggregateExprBuilder { | |
| /// Builder for physical [`AggregateExpr`] | |
| /// | |
| /// `AggregateExpr` contains the information necessary to call | |
| /// an aggregate expression. | |
| pub struct AggregateExprBuilder { |
Signed-off-by: jayzhan211 <[email protected]>
| @@ -545,6 +545,11 @@ pub mod optimizer { | |||
| pub use datafusion_optimizer::*; | |||
| } | |||
|
|
|||
| /// re-export of [`datafusion_physical_expr`] crate | |||
| pub mod physical_expr_common { | |||
There was a problem hiding this comment.
It seems other crate import *, follow them
Signed-off-by: jayzhan211 <[email protected]>
|
Thanks @alamb |
Which issue does this PR close?
Part of #11543.
Since
create_aggregate_expris not stable yet, probably will change after refactor #11359 .I only create builder API, but not mark
create_aggregate_expras deprecated for now.I also update
create_aggregate_exprto avoid breaking change onis_reversefield.Ideally, we could have
count_udaf().call_physical(args)....build()after #11359Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?