Skip to content

fix: preserve duplicate named_struct fields in codegen dispatch - #5603

Open
RRXXZZYY wants to merge 5 commits into
apache:mainfrom
RRXXZZYY:fix/named-struct-duplicate-fields-dispatch
Open

fix: preserve duplicate named_struct fields in codegen dispatch#5603
RRXXZZYY wants to merge 5 commits into
apache:mainfrom
RRXXZZYY:fix/named-struct-duplicate-fields-dispatch

Conversation

@RRXXZZYY

@RRXXZZYY RRXXZZYY commented Aug 31, 2026

Copy link
Copy Markdown

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?

  • Route duplicate-name CreateNamedStruct expressions through the JVM codegen dispatcher instead of falling the projection back to Spark.
  • Allocate affected struct subtrees with private, unique runtime child names while preserving Spark-visible field metadata where it is consumed.
  • Use duplicate-safe allocation at shared IPC, C Data import, broadcast coalescing, and C Data stream export boundaries.
  • Materialize dispatcher outputs positionally so duplicate and case-distinct direct children remain separate.
  • Add focused SQL, Arrow FFI/IPC, broadcast-join, JSON, and codegen regressions plus the expression-support documentation update.

How are these changes tested?

Validation on current head 08d43fe:

  • Spark 4.1 / Scala 2.13.17 / JDK 17 on the isolated Linux builder: main and test compilation passed.
  • CometJoinSuite / Broadcast hash join preserves duplicate struct fields: 1/1 passed with the native Comet library loaded.
  • Native Rust build: passed.
  • Earlier focused matrix coverage on this branch passed NativeUtilSuite on Spark 3.4 and 3.5 (11/11 each), create_named_struct SQL coverage on Spark 3.4 and 3.5 (1/1 each), the case-distinct from_json regression on Spark 3.4 and 3.5 (1/1 each), and combined UtilsSuite + NativeUtilSuite coverage on Spark 3.5 and 4.0 (18/18 each at the tested revisions).
  • Targeted formatting/scalastyle checks from the earlier follow-up passed; git diff --check passes on the current head.

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.

@RRXXZZYY
RRXXZZYY force-pushed the fix/named-struct-duplicate-fields-dispatch branch from 7192eb4 to 82c09a9 Compare August 31, 2026 23:43

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala
Comment thread spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala Outdated
Comment thread spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala Outdated
Comment thread spark/src/main/scala/org/apache/comet/serde/structs.scala
Comment thread docs/source/user-guide/latest/expressions.md Outdated
Comment thread spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala Outdated
Comment thread spark/src/test/scala/org/apache/comet/vector/NativeUtilSuite.scala Outdated
@RRXXZZYY
RRXXZZYY force-pushed the fix/named-struct-duplicate-fields-dispatch branch from 82c09a9 to 65581cc Compare September 1, 2026 02:30
@RRXXZZYY

RRXXZZYY commented Sep 1, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review. I addressed the feedback in 65581cc2 and rebased the branch onto current main (2949fd0d).

  • Ordinary imports now retain Arrow's original field.createVector(allocator) path. fieldForAllocation returns the original Field by identity when unchanged, and the import factory is hoisted per NativeUtil instance. A regression checks the existing $data$ list-child behavior.
  • The duplicate-safe wrappers are now limited to the affected complex subtree. RenamedStructVector uses Arrow 18.3's Field constructor; the separate construction flag is gone, with the original exported metadata exposed only after the direct children exist.
  • The internal fallback reason and doc-facing dispatcher explanation are separate, and struct is documented as Hybrid.
  • Duplicate-name integration coverage moved to create_named_struct.sql and now includes struct(a, a), nested array/map/struct values, three duplicates, all-null rows, and construction after a supported primitive-key shuffle boundary.
  • NativeUtilSuite now uses scoped resources and explicitly handles the Arrow array/schema handoff on failure paths.

Fresh validation on this head:

  • full NativeUtilSuite on Spark 4.0: 11/11 passed;
  • focused SQL-file regression on Spark 4.0, 3.5, and 3.4: passed on all three profiles;
  • focused duplicate-struct codegen test on the default Spark 4.1 profile: passed;
  • Scalastyle: 147 files, 0 errors/warnings;
  • Spotless on all four changed Scala files and git diff --check: passed.

The PR description now records the exact checks and remaining local-validation boundaries.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala Outdated
Comment thread spark/src/main/scala/org/apache/comet/serde/structs.scala

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RRXXZZYY
RRXXZZYY force-pushed the fix/named-struct-duplicate-fields-dispatch branch from c458c31 to 91bba71 Compare September 2, 2026 15:53

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

named_struct with duplicate field names falls back to Spark

3 participants