fix: preserve duplicate named_struct fields in codegen dispatch - #5603
fix: preserve duplicate named_struct fields in codegen dispatch#5603RRXXZZYY wants to merge 5 commits into
Conversation
7192eb4 to
82c09a9
Compare
andygrove
left a comment
There was a problem hiding this comment.
Thanks for taking this on. The diagnosis is right and the two unit tests are aimed at the actual mechanism, so this is close.
I checked the positional assumptions end to end and they hold: ArrayImporter.doImport walks getChildrenFromFields() positionally, CometBatchKernelCodegenOutput emits getChildByOrdinal($fi), CometScalaUDFCodegen.specFor pairs getField.getChildren.get(fi) with getChildByOrdinal(fi), and StructFieldSpec.name is only cache-key identity so duplicate names never become Java identifiers. The ordinal rename also cannot collide with a user field name, because when a struct has duplicates every child gets renamed.
The thing I would most like to see before merge is the import path staying on its old behavior when there are no duplicate names. importVector runs for every column of every batch coming back from native, and right now this rebuilds the Field tree and rewraps every complex vector regardless of whether any duplicates exist. Details inline.
One more thing worth noting: CI has not run on this branch yet. Given the local validation was Spark 4.0 only, it would be good to confirm 3.4 and 3.5 are green before merging.
82c09a9 to
65581cc
Compare
|
Thanks for the thorough review. I addressed the feedback in
Fresh validation on this head:
The PR description now records the exact checks and remaining local-validation boundaries. |
sunchao
left a comment
There was a problem hiding this comment.
Summary
The positional allocation and ordinary-import fast path address the duplicate-name case while keeping the usual import path unchanged. I found one additional P2 case in the shared codegen output allocator, attached inline.
Validation
Reviewed 65581cc2 against 2949fd0d by source, including the Arrow constructor and writer contracts. I did not execute a reproduction or rerun the author-reported tests. The four current-head workflows show action_required, so they do not establish passing CI.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed c458c31b8462fdb80a43cdff24573454ab662a2d. The previously reported import-boundary issues are addressed. No new actionable P1/P2 findings.
This was a source review. I did not run tests. Current CI still requires contributor workflow approval, so there are no passing CI results to report.
c458c31 to
91bba71
Compare
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 91bba712752eb0191efab376805a22a52a1ed785 after the base sync. No new actionable P1/P2 findings.
This was a source review. I did not run tests. Current workflows still require contributor approval, and there are no current-head or merge check results.
sunchao
left a comment
There was a problem hiding this comment.
One new P2 in the C Data stream increment, attached inline. A focused Arrow 58.4.0 probe confirmed value duplication for colliding field names. The full Spark query was source-traced, not executed. Current CI still requires workflow approval.
| vectors.add(runtimeField.createVector(allocator).asInstanceOf[FieldVector]) | ||
| ordinal += 1 | ||
| } | ||
| new VectorSchemaRoot(new Schema(runtimeFields), vectors, 0) |
There was a problem hiding this comment.
[P2] Keep stream runtime names disjoint from logical field names
Could this preserve ordinal mapping when a user names both children __comet_runtime_field_0? This root advertises __comet_runtime_field_0 and __comet_runtime_field_1. ScanStream::build_record_batch then casts back to the declared struct type, but Arrow 58.4.0 matches both target names to source child 0 and returns that child's values twice. In the existing broadcast-join regression, replacing both 'x' labels with '__comet_runtime_field_0' has this path from (1, 10) to (1, 1). A focused probe using the pinned Arrow cast reproduced the value and null duplication. I did not execute the Spark query. Please make exported runtime names disjoint from every original child name, or restore the logical schema strictly by ordinal, and cover this case in the broadcast regression.
Which issue does this PR close?
Closes #5586.
Rationale for this change
Spark permits duplicate field names in named_struct, and generated code writes those fields positionally. Arrow Java's default struct-vector factory indexes children by name, so duplicate children can collapse or reuse the wrong concrete vector type. The same behavior affects Arrow IPC and C Data stream boundaries. Arrow's struct writer also lower-cases writer-cache keys, which can drop case-distinct children such as a and A.
What changes are included in this PR?
How are these changes tested?
Validation on current head 08d43fe:
The complete CometJoinSuite was not rerun to completion on the resource-limited builder, and full repository/macOS coverage remains delegated to project CI. A full Spotless rerun was not treated as valid on the NAS SMB checkout because its Java header resource resolved as a literal path and incorrectly flagged unchanged files; no mass formatting was applied.
AI-assisted development disclosure: I used AI tooling to assist investigation and drafting. I reproduced the relevant failures, reviewed and refined the implementation, inspected the final diff and ownership paths, and ran the checks listed above.