hcrosse commented on PR #21689: URL: https://github.com/apache/datafusion/pull/21689#issuecomment-4291159750
> My concern on this approach that downstream project used to call builtin functions or spark built in functions without going through full Datafusion pipeline, and suspecting in this case the downstream users can feel benefit from this PR @comphead I checked Comet's native planner and it looks like it builds `PhysicalExprs` directly from its own protobuf serde rather than going through `SessionState::create_physical_expr`, so don't think `FunctionRewrites` apply there. Comet's Scala-side `CometConcat` serde also already gates to all-string children and falls back to Spark JVM for any array-typed concat, so `concat(array, ...)` doesn't cross into native in Comet today. So this PR shouldn't be a regression for Comet as it stands (I think 😅) If native array concat support gets added later, there are two hooks: on the Comet side, having CometConcat detect array children and emit array_concat in the protobuf and on the DataFusion side, execution-layer dispatch inside `ConcatFunc::invoke_with_args`, which needs an element-wise list concat kernel arrow-rs doesn't currently expose. [apache/arrow-rs#1772](https://github.com/apache/arrow-rs/issues/1772) tracks adding that kernel. Open to other approaches if there's one I'm missing - still trying to learn how everything pieces together! -- 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]
