waynexia commented on code in PR #8119:
URL: https://github.com/apache/arrow-datafusion/pull/8119#discussion_r1390371428
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -674,23 +677,37 @@ pub async fn from_substrait_agg_func(
args.push(arg_expr?.as_ref().clone());
}
- let fun = match extensions.get(&f.function_reference) {
- Some(function_name) => {
- aggregate_function::AggregateFunction::from_str(function_name)
- }
- None => not_impl_err!(
- "Aggregated function not found: function anchor = {:?}",
+ let Some(function_name) = extensions.get(&f.function_reference) else {
+ return plan_err!(
+ "Aggregated function not registered: function anchor = {:?}",
f.function_reference
- ),
+ );
};
- Ok(Arc::new(Expr::AggregateFunction(expr::AggregateFunction {
- fun: fun.unwrap(),
- args,
- distinct,
- filter,
- order_by,
- })))
+ // try udaf first, then built-in aggr fn.
Review Comment:
I was considering putting this logic into `SessionContext`, as well as
resolving scalar and window functions. However `sql_function_to_expr` uses
`ContextProvider` to do this resolving. Maybe we have to do some refactors on
`ContextProvider` at first.
By the way, I noticed that `ContextProvider` has lots of mock impl but those
function resolving methods are only used in `sql_function_to_expr`. It also
looks like an indication that refactoring is needed to me.
--
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]
