Blizzara commented on code in PR #13803:
URL: https://github.com/apache/datafusion/pull/13803#discussion_r1890163208
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1503,370 +1858,416 @@ pub async fn from_substrait_func_args(
/// Convert Substrait AggregateFunction to DataFusion Expr
pub async fn from_substrait_agg_func(
- state: &dyn SubstraitPlanningState,
+ consumer: &impl SubstraitConsumer,
f: &AggregateFunction,
input_schema: &DFSchema,
- extensions: &Extensions,
filter: Option<Box<Expr>>,
order_by: Option<Vec<SortExpr>>,
distinct: bool,
) -> Result<Arc<Expr>> {
- let args =
- from_substrait_func_args(state, &f.arguments, input_schema,
extensions).await?;
-
- let Some(function_name) = extensions.functions.get(&f.function_reference)
else {
+ let Some(fn_signature) = consumer
+ .get_extensions()
Review Comment:
> scalar functions first try and lookup the function by name, then look it
up as an operator and then try and treat is a BuiltinExpr.
Yeah, I think this is a side-effect of DF not having (yet?) converged fully
on UDFs. I think for aggregates and windows the built-in modes were already
removed in favor of UDAF/UDWF but not for scalars.
But yep, totally good to do as a followup someday!
--
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]
