sunchao commented on PR #5603: URL: https://github.com/apache/datafusion-comet/pull/5603#issuecomment-5605022239
**Yes. I’d prefer enabling the existing native constructor while retaining the Arrow boundary fixes.** ### Much of the native support already exists The Rust `CreateNamedStruct` already pairs field names with child arrays **by position** and constructs a `StructArray`. Neither that constructor nor its native planner rejects duplicate names. The explicit rejection is in the Scala serializer. [Native constructor](https://github.com/apache/datafusion-comet/blob/95fad218e12c6fda4500d860e41512e76390319a/native/spark-expr/src/struct_funcs/create_named_struct.rs#L41), [Scala guard](https://github.com/apache/datafusion-comet/blob/95fad218e12c6fda4500d860e41512e76390319a/spark/src/main/scala/org/apache/comet/serde/structs.scala#L45). So this looks primarily like **enabling and validating an existing capability**, rather than writing a new native implementation. ### Why I prefer that For: ```sql named_struct('x', a + 1, 'x', b * 2) ``` The PR’s dispatcher executes the whole expression on the JVM. A native route would allow both arithmetic children and struct construction to stay native. That should eliminate concrete work: - The JVM callback for this constructor. - Spark’s per-row struct-object allocation. - Copying those row fields back into Arrow output vectors. For existing column arrays, native struct construction can reuse the child arrays; scalar children may still need expansion. These are source-based advantages, **not a measured query speedup**. ### The Arrow repairs still matter Native-produced duplicate structs still reach Java when returning results, shuffling, broadcasting, or invoking JVM expressions. Arrow Java would encounter the same name-collision problem there. Therefore, native support still needs the duplicate-safe import, IPC, broadcast, and stream-export changes. The shared codegen allocation repair also remains useful for other JVM expressions that return structs. [Shared allocation changes](https://github.com/apache/datafusion-comet/blob/95fad218e12c6fda4500d860e41512e76390319a/spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala#L335). ### My recommendation **Make duplicate-name construction native-first.** Use JVM dispatch where child expressions actually require it, rather than making duplicate labels themselves trigger dispatch. Before accepting that alternative, I’d run the boundary regressions with struct dispatch disabled and assert native execution, including mixed types, nulls, scalar/column mixtures, nested structs, shuffle, and broadcast. The source makes this approach look preferable and feasible. **I haven’t tested that planner change end to end; the current PR’s passing CI validates its dispatcher-based implementation.** -- 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]
