goldmedal commented on code in PR #10917:
URL: https://github.com/apache/datafusion/pull/10917#discussion_r1641265848
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -496,11 +496,14 @@ impl AsExecutionPlan for protobuf::PhysicalPlanNode {
}
AggregateFunction::UserDefinedAggrFunction(udaf_name) => {
let agg_udf =
registry.udaf(udaf_name)?;
+ // TODO: 'logical_exprs' is not
supported for UDAF yet.
+ // approx_percentile_cont and
approx_percentile_cont_weight are not supported for UDAF from protobuf yet.
+ let logical_exprs = &[];
Review Comment:
Yes, it will be correct for the logical plan. However, I think it will cause
some issues when processing the physical plan. Curiously, should we expect the
physical roundtrip to work? I tried to add a roundtrip test for it
in`datafusion/proto/tests/cases/roundtrip_physical_plan.rs` like:
```rust
#[test]
fn roundtrip_approx_percentile_cont_udaf() -> Result<()> {
let field_a = Field::new("a", DataType::Int64, false);
let field_b = Field::new("b", DataType::Float64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
let udaf = (*approx_percentile_cont_udaf()).clone();
let ctx = SessionContext::new();
ctx.register_udaf(udaf.clone());
let groups: Vec<(Arc<dyn PhysicalExpr>, String)> =
vec![(col("a", &schema)?, "unused".to_string())];
let aggregates: Vec<Arc<dyn AggregateExpr>> =
vec![udaf::create_aggregate_expr(
&udaf,
&[col("b", &schema)?, lit(ScalarValue::Float64(Some(0.5)))],
&[datafusion_expr::col("b"),
datafusion_expr::lit(ScalarValue::Float64(Some(0.5)))],
&[],
&[],
&schema,
"approx_percentile_cont_agg",
false,
false,
)?];
roundtrip_test_with_context(
Arc::new(AggregateExec::try_new(
AggregateMode::Final,
PhysicalGroupBy::new_single(groups.clone()),
aggregates.clone(),
vec![None],
Arc::new(EmptyExec::new(schema.clone())),
schema,
)?),
&ctx,
)
}
```
It will lose the logical_plan when converting back to the physical plan from
protobuf.
Is this test necessary? I only see an physical-plan roudtrip testing of udaf
called `roundtrip_aggregate_udaf`.
--
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]
