You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "dharanad (via GitHub)" <gi...@apache.org> on 2024/06/17 19:13:02 UTC
[PR] Convert Average to UDAF #10942 [datafusion]
dharanad opened a new pull request, #10964:
URL: https://github.com/apache/datafusion/pull/10964
## Which issue does this PR close?
Closes #10942
## Rationale for this change
Part of #8708
## What changes are included in this PR?
## Are these changes tested?
```
cargo build
cargo test
cargo test --test sqllogictests
```
## Are there any user-facing changes?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Convert Average to UDAF #10942 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10964:
URL: https://github.com/apache/datafusion/pull/10964
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Convert Average to UDAF #10942 [datafusion]
Posted by "dharanad (via GitHub)" <gi...@apache.org>.
dharanad commented on code in PR #10964:
URL: https://github.com/apache/datafusion/pull/10964#discussion_r1649703615
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -281,17 +283,6 @@ fn roundtrip_window() -> Result<()> {
Arc::new(window_frame),
));
- let plain_aggr_window_expr = Arc::new(PlainAggregateWindowExpr::new(
Review Comment:
My bad, i might have remove it while refactoring it. But the test case is failing with below error.
```
thread 'cases::roundtrip_physical_plan::roundtrip_window' panicked at datafusion/proto/tests/cases/roundtrip_physical_plan.rs:113:14:
to proto: NotImplemented("Aggregate function not supported: AggregateFunctionExpr { fun: AggregateUDF { inner: Avg { signature: Signature { type_signature: UserDefined, volatility: Immutable }, aliases: [\"mean\"] } }, args: [Column { name: \"b\", index: 1 }], logical_args: [], data_type: Float64, name: \"AVG(b)\", schema: Schema { fields: [Field { name: \"a\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: \"b\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, sort_exprs: [], ordering_req: [], ignore_nulls: false, ordering_fields: [], is_distinct: false, input_type: Int64 }")
```
This is occuring because we have removed
```rust
fn aggr_expr_to_aggr_fn(expr: &dyn AggregateExpr) -> Result<AggrFn> {
...
} else if aggr_expr.downcast_ref::<Avg>().is_some() {
protobuf::AggregateFunction::Avg
}
...
```
I thinking we can remove this test case from the physical plan and include it in the logical plan instead?
I'm still learning about query engines, so please forgive me if my understanding is incomplete
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -281,17 +283,6 @@ fn roundtrip_window() -> Result<()> {
Arc::new(window_frame),
));
- let plain_aggr_window_expr = Arc::new(PlainAggregateWindowExpr::new(
Review Comment:
@jayzhan211 My bad, i might have removed it while refactoring it. But the test case is failing with below error.
```
thread 'cases::roundtrip_physical_plan::roundtrip_window' panicked at datafusion/proto/tests/cases/roundtrip_physical_plan.rs:113:14:
to proto: NotImplemented("Aggregate function not supported: AggregateFunctionExpr { fun: AggregateUDF { inner: Avg { signature: Signature { type_signature: UserDefined, volatility: Immutable }, aliases: [\"mean\"] } }, args: [Column { name: \"b\", index: 1 }], logical_args: [], data_type: Float64, name: \"AVG(b)\", schema: Schema { fields: [Field { name: \"a\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: \"b\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, sort_exprs: [], ordering_req: [], ignore_nulls: false, ordering_fields: [], is_distinct: false, input_type: Int64 }")
```
This is occuring because we have removed
```rust
fn aggr_expr_to_aggr_fn(expr: &dyn AggregateExpr) -> Result<AggrFn> {
...
} else if aggr_expr.downcast_ref::<Avg>().is_some() {
protobuf::AggregateFunction::Avg
}
...
```
I thinking we can remove this test case from the physical plan and include it in the logical plan instead?
I'm still learning about query engines, so please forgive me if my understanding is incomplete
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -281,17 +283,6 @@ fn roundtrip_window() -> Result<()> {
Arc::new(window_frame),
));
- let plain_aggr_window_expr = Arc::new(PlainAggregateWindowExpr::new(
Review Comment:
I have added a test case in `fn roundtrip_window` in `datafusion/proto/tests/cases/roundtrip_logical_plan.rs`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Convert Average to UDAF #10942 [datafusion]
Posted by "dharanad (via GitHub)" <gi...@apache.org>.
dharanad commented on PR #10964:
URL: https://github.com/apache/datafusion/pull/10964#issuecomment-2182123378
> > * test_drop_cancel_with_groups
>
> I think you can try `create_aggregate_expr` in `datafusion/physical-expr-common/src/aggregate/mod.rs` to create avg expression
I am using the same to create avg expression. But it is still failing
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org