vbarua commented on code in PR #13803:
URL: https://github.com/apache/datafusion/pull/13803#discussion_r1889342502
##########
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:
I was trying to figure out the right abstraction not just for resolving the
function via the extension, but also mapping it into DataFusion. However I
found the existing logic a little unwieldy to reason about. Specifically 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.
I want to think about function mapping in isolation, because I suspect it
will be easier to figure out the right abstraction if we simplify the lookup
logic first.
--
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]
