comphead commented on PR #21689: URL: https://github.com/apache/datafusion/pull/21689#issuecomment-4297839039
> 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! Sorry folks, I still not very confident with this change. From Comet perspective you totally right, Comet typically has patches to be Spark compliant, some times going ahead and then backport changes to DataFusion, but there are also other Spark based project like LakeSail and other folks that directly replace functions with DF counterparties and my expectation would be them also benefit on this function. My understanding the implementation should be pretty straightforward, just delegate the call to array_concat if incoming arguments for `concat` are lists, however there is a fundamental issue with dependencies, namely: The problem: `ConcatFunc` (string) and `ArrayConcat` live in separate crates with a one-way dependency (functions-nested → functions), so adding a reverse dependency would be circular. That means `ConcatFunc::invoke_with_args` can't call `array_concat_inner` directly. I think we need to investigate how to overcome this issue. -- 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]
