Skip to content

fix: fall back when a struct repeats a Parquet field id - #6004

Merged
comphead merged 1 commit into
apache:mainfrom
comphead:fix/duplicate-struct-fields-fallback
Sep 18, 2026
Merged

comphead merged 1 commit into
apache:mainfrom
comphead:fix/duplicate-struct-fields-fallback

Conversation

@comphead

@comphead comphead commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5801.

Rationale for this change

Comet has to match Spark. Under spark.sql.parquet.fieldId.read.enabled Spark resolves each requested field to the one Parquet field carrying its id, and raises FOUND_DUPLICATE_FIELD_IN_FIELD_ID_LOOKUP_MODE when more than one answers. Comet never looked at field ids at all, so a requested struct that repeats an id was read positionally and returned rows where Spark raises.

The ids are in the requested schema's StructField.metadata, so this is decidable from the plan. The fix is a schema check and a fallback, nothing more.

What changes are included in this PR?

  1. DataTypeSupport gains hasDuplicateFieldIds, a predicate beside the existing duplicate-name one. The existing name check is refactored to call the shared hasDuplicateFieldNames so the two read the same way.
  2. CometScanTypeChecker declines a struct that repeats a field id, when field id matching is on. DataTypeSupport's recursion carries the check into nested structs, arrays and maps, so the checker itself stays a shallow predicate.

That is the whole change: check the schema, fall back. No Parquet decoding, no footer reads, no operator-level gates.

How are these changes tested?

  • DataTypeSupportSuite (new): both predicates, including that names differing only by case are not duplicates (Java Arrow tells a and A apart), that a field with no id cannot collide, and that a non-integral id is left for Spark's ParquetUtils.getFieldId to reject rather than misreported as a duplicate.
  • CometScanRuleSuite: the checker declines a duplicate-id struct at every nesting level (struct child, array element, map value), and accepts all of them again with spark.sql.parquet.fieldId.read.enabled=false, where the fields are told apart by name.
  • CometNativeReaderSuite: end to end. A Parquet file with no key-value metadata and schema s<x id=1, y id=1>, read back with that same schema — which is what makes DataFusion's opener skip the expression adapter. Comet now raises Spark's Found duplicate field(s) "1": [x, y] in id mapping mode and the plan carries no CometNativeScanExec. With field id matching off the scan stays native and returns the row.

Run locally on the Spark 4.1 profile: DataTypeSupportSuite, CometScanRuleSuite, CometExecRuleSuite, CometNativeReaderSuite, CometNativeShuffleSuite, CometInMemoryCacheSuite, CometShuffleSuite, DisableAQECometShuffleSuite, ParquetReadV1Suite, CometFuzzTestSuite. cargo fmt --check and cargo clippy --all-targets --workspace -- -D warnings clean (no Rust changes).

Note on #5783, and a warning for #5786

While checking whether this PR could also cover #5783, I compared four readers on the same file. It turns out Comet has no duplicate-struct-field-name bug, and the guard #5786 proposes would cause a regression.

named_struct('dup', id, 'dup', id + 100) written by Spark produces a file that is internally inconsistent — the row group declares 3 rows while each of the two same-path s.dup leaf chunks declares 6 values:

path_in_schema   num_values   row_group_num_rows
s, dup           6            3
s, dup           6            3

parquet-mr cannot read it at all (ParquetEncodingException: [s, dup] -(0)-> 2). Spark and DuckDB clamp to 3 rows and return 0, 100, 1, which is the two children interleaved, not the original data. Comet follows num_values and returns 6. So there is no correct answer to match on that file; Spark's row count is right and its values are not.

Writing the same logical data with a correct writer shows Comet is fine:

File written by num_values / num_rows Spark DuckDB DataFusion 55 Comet
Spark 6 / 3 3 rows, 0,100,1 3 rows, same 6 rows (count(*) says 3) 6 rows
DataFusion 3 / 3 3 rows, correct 3 rows, correct 3 rows, correct 3 rows, correct
DuckDB (renames to dup_1) 3 / 3 3 rows, correct 3 rows, correct 3 rows, correct 3 rows, correct

On the DataFusion-written file — same duplicate dup/dup struct, same spark.read.schema("s struct<dup: bigint>") read — Spark and Comet both return [0], [1], [2] with CometNativeScanExec in the plan. Exact match. DuckDB refuses to construct the shape at all (Binder Error: Duplicate struct entry name "dup").

Two things follow:

Separately, DataFusion answers count(*) from row-group metadata and the scan from num_values, so on the malformed file it returns 3 and 6 in the same session, even with the scan wrapped in a subquery. That is worth an upstream issue and is independent of Comet.

@comphead comphead added the run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue label Sep 17, 2026
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Sep 17, 2026
@andygrove

Copy link
Copy Markdown
Member

@comphead is this different from #5786?

@comphead

comphead commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

@comphead is this different from #5786?

this PR is supposed to be more generic, not only Parquet scan, but if UDF or any operator produces a schema with a struct having duplicated column we gonna fallback. Other PRs likely to be built on top of this? WDYT? @andygrove @dwsmith1983 @ErikBPF

I expect the change should be straightforward as we know about the schema attached to the plan nodes on the planning side.

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

Correctness

This change extends duplicate-struct fallback to operator conversion and the Arrow cache serializer. Previously the shared type checker protected scans, shuffle, and row conversion, but an operator or cache could still expose a struct with repeated child names to Java Arrow. The new operator check covers both outputs and data-producing inputs, and the cache check consistently selects Spark's default cache format for unsupported structs.

Maintained Spark 3.5 and 4.0 resolve Parquet field IDs among siblings and raise when a requested ID matches multiple physical fields. That rule also applies at the file-schema root. The new scan guard catches nested duplicates but misses duplicate IDs on top-level fields, because the inherited schema entry point passes only each field's data type. This leaves a declared-schema case of the same equal-schema/no-predicate reader bypass unguarded. The first inline P2 describes the missing root check and regression case.

The second P2 is the missing CI registration for DataTypeSupportSuite. Both preflight runs fail on that omission, and the Spark runtime jobs are skipped. The new reader fixture usefully covers metadata-free files with field-ID matching enabled and disabled, while the cache test checks the selected batch format and readback. Those runtime results are author-reported, not independently established here.

Validation and scope

Reviewed head 204b6574 against base 74725611. The seven authored file changes are identical in merge 3dc05091, which also includes the newer base shuffle commit. Local python3 -B dev/ci/check-suites.py reproduces the preflight failure. An isolated compile of the exact shared type checker and extracted scan checker confirms that top-level duplicate IDs are accepted while nested duplicate IDs are rejected. That probe uses configuration/type-shim stubs and cached Spark type classes. Its later flag-disabled control stops on a missing Kryo class, so this is component evidence, not a passing Spark/JNI suite.

The maintained Spark 3.4 and 4.1 branches were unavailable for semantic comparison. Physical-only duplicate names hidden by a declared schema remain outside this PR's stated scope.

Performance

The added checks run during planning and cache-format selection rather than per row. Falling back prevents constructing Arrow batches whose struct children cannot be represented faithfully. The operator check short-circuits after the first duplicate and preserves ordinary repeated top-level output names.

The scan check recursively walks a type and then the existing support checker recursively visits it again, so deeply nested schemas repeat some work. Moving the ID validation to a single schema-level pass would also address the correctness gap. I have no benchmark evidence of a material planning regression, and no execution-speed claim follows from this review.

Design

A common conversion gate is a sensible place to enforce the Arrow boundary invariant. Inspecting child outputs matters for operators that consume a duplicate-named struct without returning it, and unwrapping WriteFilesExec follows the existing input handling. Using the serializer's support predicate for both cache creation and reading keeps the chosen cache format consistent.

Name duplication and Parquet ID ambiguity need different entry points. Repeated top-level output names can be valid positional columns, but duplicate field IDs at the file root still require the Parquet read check. Keep the current nested-name behavior and apply ID validation to the complete requested scan schema when field-ID matching is enabled.

Abstraction & complexity

The small recursive helper keeps struct, array, and map traversal together and produces a useful path in fallback explanations. It does not require a new framework or configuration surface. Malformed field-ID metadata is excluded from duplicate detection, while the existing Spark-backed schema serialization still validates the ID value.

The main simplification is to validate field IDs once at the schema boundary instead of recursively re-entering the same check through each type. The two inline P2s are the actionable changes from this review.

// raising, so hand the read back to Spark and let it report the ambiguity. See #5801.
lazy val duplicateFieldIds =
if (CometParquetUtils.readFieldId(SQLConf.get)) {
DataTypeSupport.findDuplicateStructFieldIds(dt, name)

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.

Correctness

[P2] Check duplicate field IDs on the complete requested schema

DataTypeSupport.isSchemaSupported calls this method with each field's dataType, so StructType(x: Long id=1, y: Long id=1) reaches this helper twice as LongType and is accepted. I confirmed that with an isolated compile of the exact checker. Wrapping the same fields inside s correctly falls back.

For a metadata-free Parquet file with those two top-level fields and the same requested schema, this leaves the same no-predicate bypass as the new nested fixture: DataFusion 55.1 skips the expression adapter when the schemas are equal. Spark's field-ID ambiguity check applies at the root as well. Please run this check once on the complete requested schema and add the flattened version of the new reader fixture. The exception for repeated top-level output names does not apply to Parquet field IDs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7fe9328, and thank you both — this was a real gap, not a nit. @andygrove's run at the root is exactly right, and the cause is structural as he says: isSchemaSupported hands isTypeSupported each field's data type, so the schema's own field list is never available to any override.

I took a slightly different shape than "run the check once over the whole requested schema", so flagging it for you to sanity-check. Rather than one recursive pass, CometScanTypeChecker now overrides both entry points and each does one shallow check of exactly one field list:

  • isSchemaSupported → the schema's own fields (the root case that was missing)
  • isTypeSupported → each nested struct, as the trait's existing recursion reaches it

Same coverage, and every field list is visited exactly once, so it also removes the repeated subtree traversal @sunchao flagged under Performance.

Tests, both as requested:

  • CometScanRuleSuite now leads with the unwrapped root schema, alongside a wrapped struct and a map value, plus distinct-id / one-id-absent / malformed-id acceptance controls and the flag-disabled control for each.
  • CometNativeReaderSuite has the flattened fixture as its own test: message spark_schema { optional int64 x = 1; optional int64 y = 1; }, no key-value metadata, read back with the matching schema. Comet declines and Spark reports Found duplicate field(s) "1"; with field id matching off the scan stays native.

One more fix while in here: the root reason was rendering doubled, because the caller already wraps reasons in s"Unsupported schema ${requiredSchema}: ...". Dropped the redundant prefix.

I did consider the deeper fix of adding a field-list hook to DataTypeSupport itself so every checker gets one, and decided against it here. It needs an isRoot flag whose only job is to suppress the duplicate-name check at the root — top-level output attributes legitimately repeat a name, a self-join gives two id columns and Comet matches those positionally — and that risks silently changing behaviour for the shuffle, row-conversion and cache checkers in order to benefit one subclass. Happy to do it if you'd rather.

* Arrow tell these struct children apart, and can Spark's Parquet reader resolve them one-to-one
* -- so the answers live in one place and are pinned here.
*/
class DataTypeSupportSuite extends AnyFunSuite {

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.

Correctness

[P2] Register the new suite in both CI workflow matrices

dev/ci/check-suites.py requires every non-excluded suite to appear in both .github/workflows/pr_build_linux.yml and .github/workflows/pr_build_macos.yml. Neither contains org.apache.comet.DataTypeSupportSuite. The current preflight exits 255 with Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.DataTypeSupportSuite, which I also reproduced locally. This stops CI before the runtime test jobs run. Please add the suite to both matrices so the guard succeeds and the new unit tests execute.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 7fe9328, but by removing the suite rather than registering it — flagging that explicitly since you'd already verified the registration as fixed and I don't want this to read as a regression.

A cleanup pass found DataTypeSupportSuite largely duplicated CometNativeShuffleSuite:712, which already asserts duplicate names through isSchemaSupported, the array/map recursion, and the a-vs-A negative. Only two assertions were genuinely new — the malformed field id and the mixed present/absent id — and both moved into the CometScanRuleSuite table.

So the coverage still runs in CI: org.apache.comet.rules.CometScanRuleSuite is registered in both matrices already, and it exercises the predicates through the real entry point rather than directly. check-suites.py passes, one file and two registrations fewer.

Your underlying point stands and was the right catch — a new suite that CI never runs is worse than no suite.

@comphead
comphead force-pushed the fix/duplicate-struct-fields-fallback branch from 204b657 to a406fc7 Compare September 17, 2026 19:45
@comphead comphead changed the title fix: decline structs with duplicate field names or Parquet field ids fix: decline Parquet scans whose struct repeats a field id Sep 17, 2026

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

The force-push changed what this PR is, so flagging that up front. The operator conversion gate is gone, which means the "not only Parquet scan, any operator that produces a duplicate-named struct falls back" part that @comphead described above as the difference from #5786 is no longer in here. What is left is the scan check for duplicate field ids plus the cache serializer check. That is a reasonable PR on its own, but it means the "any operator" half of #5605 is now covered by nothing that is open. Is that coming back here, or should we file it separately so it does not get lost?

One note if it does come back. I ran the earlier head 204b6574 and that gate broke CometArrayExpressionSuite "folded map value with duplicate struct field names falls back (multirow)". The plan still falls back and the answer is still right, but the gate fires before the serde runs, so the recorded reason becomes Native operators do not support v.map value: struct with duplicate field names (x) and the expected Unsupported data type MapType is never recorded. That suite was not in the list you ran locally.

Preflight is red on both runs with Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.DataTypeSupportSuite. dev/ci/check-suites.py scans pr_build_macos.yml too, so it needs adding to both. Everything downstream is skipped right now, so there is no runtime coverage of this change in CI yet.

One more small thing that did not fit on a line in the diff. The description says the point of hasDuplicateFieldNames is that the definition of "duplicate name" lives in one place, but CometShuffleExchangeExec.scala:483 and CometShuffleExchangeExec.scala:610 still spell it out by hand as fields.map(f => f.name).distinct.length == fields.length. Could those use the new helper while you are in here?

I ran this locally on the Spark 4.1 profile, JDK 17, macOS aarch64, at a406fc75. Both of your new runtime tests pass, and so does CometExpressionSuite "named_struct with duplicate field names". The root-level field id case in my inline comment below fails.

// carrying its id, and raises when more than one answers. A requested struct that repeats an
// id cannot be resolved that way, and the native scan reads it positionally rather than
// raising, so hand the read back to Spark and let it report the ambiguity. See #5801.
lazy val duplicateFieldIds =

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.

I confirmed @sunchao's point about top-level fields with a real run rather than a compile probe, and it does reproduce on this head.

Same construction as your new test in CometNativeReaderSuite, but with the two id-1 fields at the top level instead of inside s:

message spark_schema {
  optional int64 x = 1;
  optional int64 y = 1;
}

read back with StructType(Seq(withFieldId("x", 1), withFieldId("y", 1))) and spark.sql.parquet.fieldId.read.enabled=true:

COMET: ROWS=[10,20]      (CometNativeScan in the plan)
SPARK: ERROR=Found duplicate field(s) "1": [x, y] in id mapping mode.

That is #5801 exactly, one level up. The reason is structural rather than an oversight in the predicate: DataTypeSupport.isSchemaSupported hands isTypeSupported each field's dataType, so the root struct's fields are never compared against each other and this override cannot see them.

Would you consider overriding isSchemaSupported here instead and running findDuplicateStructFieldIds once over the whole requested schema? That covers the root, and it also drops the repeated traversal, since findDuplicateStructFieldIds already recurses while isTypeSupported re-enters it at every nesting level. Could the root case get a test alongside the nested one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7fe9328, and thank you both — this was a real gap, not a nit. @andygrove's run at the root is exactly right, and the cause is structural as he says: isSchemaSupported hands isTypeSupported each field's data type, so the schema's own field list is never available to any override.

I took a slightly different shape than "run the check once over the whole requested schema", so flagging it for you to sanity-check. Rather than one recursive pass, CometScanTypeChecker now overrides both entry points and each does one shallow check of exactly one field list:

  • isSchemaSupported → the schema's own fields (the root case that was missing)
  • isTypeSupported → each nested struct, as the trait's existing recursion reaches it

Same coverage, and every field list is visited exactly once, so it also removes the repeated subtree traversal @sunchao flagged under Performance.

Tests, both as requested:

  • CometScanRuleSuite now leads with the unwrapped root schema, alongside a wrapped struct and a map value, plus distinct-id / one-id-absent / malformed-id acceptance controls and the flag-disabled control for each.
  • CometNativeReaderSuite has the flattened fixture as its own test: message spark_schema { optional int64 x = 1; optional int64 y = 1; }, no key-value metadata, read back with the matching schema. Comet declines and Spark reports Found duplicate field(s) "1"; with field id matching off the scan stays native.

One more fix while in here: the root reason was rendering doubled, because the caller already wraps reasons in s"Unsupported schema ${requiredSchema}: ...". Dropped the redundant prefix.

I did consider the deeper fix of adding a field-list hook to DataTypeSupport itself so every checker gets one, and decided against it here. It needs an isRoot flag whose only job is to suppress the duplicate-name check at the root — top-level output attributes legitimately repeat a name, a self-join gives two id columns and Comet matches those positionally — and that risks silently changing behaviour for the shuffle, row-conversion and cache checkers in order to benefit one subclass. Happy to do it if you'd rather.

fields.map(_.name).distinct.length != fields.length

/**
* Describes the first struct nested anywhere in `dt` whose children repeat a Parquet field id,

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.

Small thing about this doc comment. ParquetReadSupport.matchIdField raises when the id a requested field asks for matches more than one field in the file, whereas this checks whether two requested fields share an id. Those are different predicates, and they only coincide under the equal-schema condition #5801 describes.

That is enough for the gap you are closing, but the comment reads as though the second follows from the first, and someone will rely on that later. Could it say which one is implemented and why the narrower check is sufficient here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, and you're right that the comment implied the second followed from the first. Rewritten in 7fe9328 to lead with the distinction:

 * True when two of `fields` declare the same Parquet field id.
 *
 * Deliberately not Spark's check: `ParquetReadSupport.matchIdField` raises when one *requested*
 * id is carried by several fields *in the file*. The two coincide only when the requested
 * schema equals the file schema, which is exactly when DataFusion's opener skips the expression
 * adapter that would have validated the lookup (#5801). So this can decline a read Spark would
 * have accepted, costing native execution but not correctness; it cannot report a duplicate
 * Spark would not. File-side ambiguity is left to #5786.
 *
 * Only meaningful under `spark.sql.parquet.fieldId.read.enabled`; callers gate on that.

It now says which predicate is implemented, why the narrower one is sufficient here, and which direction the error can go — it can cost native execution but not correctness, and it cannot invent a duplicate Spark would not report.

val MAP_VALUE = "map value"

/** Spark's `StructField` metadata key for a Parquet field id. */
private val FIELD_ID_METADATA_KEY = "parquet.field.id"

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.

Comet already uses ParquetUtils.hasFieldId and ParquetUtils.getFieldId for this in serde/operator/package.scala:48-51 and QueryPlanSerde.scala:700. Any reason not to use ParquetUtils.FIELD_ID_METADATA_KEY and ParquetUtils.hasFieldId here rather than redeclaring the key and hand-rolling the metadata read? It would keep the two from drifting if Spark ever changes it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No good reason — done in 7fe9328. fieldId now uses ParquetUtils.hasFieldId and ParquetUtils.getFieldId, and the redeclared FIELD_ID_METADATA_KEY is gone.

The only wrinkle is that getFieldId raises on a non-integral or out-of-range id, and this runs during planning where raising would be the wrong error, so it is wrapped:

private def fieldId(field: StructField): Option[Int] = {
  if (!ParquetUtils.hasFieldId(field)) None
  else {
    // A malformed id is not this check's business to report -- getFieldId raises on one -- so
    // treat it as absent and let the reader complain about it.
    try Some(ParquetUtils.getFieldId(field))
    catch { case _: IllegalArgumentException => None }
  }
}

Still Spark's own definition of what a field id is, which is the drift you were guarding against. There's a malformed id case in the CometScanRuleSuite acceptance controls pinning that it is not misreported as a duplicate.

Related, from the same cleanup: hasDuplicateFieldNames was extracted to be shared, but two inline copies of fields.map(f => f.name).distinct.length == fields.length were still sitting in CometShuffleExchangeExec at lines 483 and 610. Those now call the shared predicate. I left CometBatchKernelCodegen:103 alone — it works off st.fieldNames with a deliberate comment about not rebuilding that array, and converting it would reintroduce the allocation the comment avoids.

@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 a406fc75 against 3b942e59. The generic operator conversion gate is removed, while the Parquet field-ID guard and Arrow cache fallback remain. The ID helper is simpler and its unit cases now include map keys and deeper nesting. I found no new P1/P2 issues in this narrower scope.

Both published P2s remain:

  • Root field IDs: isSchemaSupported still passes each field's type separately, so the root IDs are never compared. The exact-checker probe accepts root duplicates with field-ID reading enabled, rejects nested duplicates, and accepts distinct IDs and flag-disabled controls. The native adapter remains bypassed for equal schemas without a predicate.
  • Suite registration: DataTypeSupportSuite still exists and is absent from both workflow matrices. Local check-suites.py exits 255, matching current preflight.

The probe passed eight schema-entry controls and five helper checks using cached Spark type classes and configuration/type-shim stubs. This is component evidence, not a Spark/JNI run. Maintained Spark 3.5/4.0 still require the ambiguity error at the root. Maintained 3.4/4.1 sources were unavailable. CI checked merge 09ec1e57, which preserves all six authored files, but preflight stopped it before runtime tests.

@comphead

Copy link
Copy Markdown
Contributor Author

#5786 we would still need it, the rabbit hole is deeper than I expected. Claude led me to Spark writer issues which can be investigated and fixed later.

@comphead
comphead force-pushed the fix/duplicate-struct-fields-fallback branch from a406fc7 to 3df2a8b Compare September 17, 2026 20:59
@comphead comphead changed the title fix: decline Parquet scans whose struct repeats a field id fix: fall back when a struct repeats a Parquet field id Sep 17, 2026
@comphead

Copy link
Copy Markdown
Contributor Author

Created DF issue apache/datafusion#25433

@comphead
comphead force-pushed the fix/duplicate-struct-fields-fallback branch from 3df2a8b to 806d909 Compare September 17, 2026 21:22

@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 806d9098 against d0c10f2d. The ID helper is now a shallow predicate using the existing type recursion, which removes repeated subtree traversal. The new scan-rule tests cover wrapped structs, arrays and map values. I found no new P1/P2 issues.

The suite-registration P2 is fixed. DataTypeSupportSuite is listed in both workflow matrices, local check-suites.py passes, and current preflight passes on merge f680e09b.

The P2 for root field IDs remains. isSchemaSupported still passes each field's type separately, so root IDs are never compared. Every new test shape is wrapped inside col. The fresh exact-checker probe accepts root duplicates with field-ID reading enabled and rejects nested duplicates. The schema-level check and flattened reader fixture are still needed.

The probe passed 16 schema-entry controls and five helper checks, including map keys, deeper nesting, distinct IDs and flag-disabled cases. This is component evidence with configuration/type-shim stubs and cached Spark type classes, not a full Spark/JNI run. Maintained Spark 3.5/4.0 still require the root ambiguity error. Maintained 3.4/4.1 sources were unavailable. Runtime CI is still in progress. The tested merge retains the seven source/test files exactly and both workflow registrations alongside newer base suites.

Under `spark.sql.parquet.fieldId.read.enabled` Spark resolves each
requested field to the one Parquet field carrying its id, and raises
FOUND_DUPLICATE_FIELD_IN_FIELD_ID_LOOKUP_MODE when more than one
answers. Comet never looked at field ids, so a requested schema that
repeats one was read positionally and returned rows where Spark raises.

The ids ride in the requested schema's `StructField.metadata`, so this
is decidable from the plan. `DataTypeSupport` gains a
`hasDuplicateFieldIds` predicate beside the existing duplicate-name one,
and `CometScanTypeChecker` declines a field list that trips it.

Both entry points are overridden, because they see different things.
`isTypeSupported` is handed each field's data type, so it sees nested
structs as the trait's recursion reaches them but can never compare two
top-level fields; `isSchemaSupported` is where the schema's own field
list is available. Each list is inspected exactly once, so nothing is
re-walked at an enclosing level.

`ArrowCachedBatchSerializer.supportsType` also accepted a struct with
duplicate child names, so caching such a relation stored it in Comet's
Arrow format, which Java Arrow cannot import back because it keys struct
children by name. One more schema check delegates it to Spark's default
cache format, alongside the interval types already excluded there, and
the two copies of that predicate in the shuffle gate now call the shared
one rather than spelling it out inline.

No Parquet decoding changes; see apache#5786 for that path.

Closes apache#5801.
@comphead
comphead force-pushed the fix/duplicate-struct-fields-fallback branch from 806d909 to 7fe9328 Compare September 18, 2026 00:25
@github-actions github-actions Bot added the area:shuffle Shuffle (JVM and native) label Sep 18, 2026
@comphead

Copy link
Copy Markdown
Contributor Author

@andygrove on the operator gate — it is intentionally out of this PR, and it should be filed separately so it does not get lost. I'd rather not bring it back here: this PR is now just "check the schema, fall back", and the operator gate is a different mechanism with a different blast radius.

Your CometArrayExpressionSuite finding is a good argument for keeping them apart, and thank you for running it — that suite was not in what I ran locally. Confirmed: the gate fired before the serde, so the recorded reason became Native operators do not support v.map value: struct with duplicate field names (x) and the expected Unsupported data type MapType was never recorded. Right answer, wrong reason, and the test rightly cared. That suite is green on this head.

Worth recording what I found while checking whether this PR could also cover #5783, because it changes what a fix there should aim for. I compared four readers on the file Spark's writer produces for named_struct('dup', id, 'dup', id + 100):

File written by chunk num_values / row group num_rows Spark DuckDB DataFusion 55 Comet
Spark 6 / 3 3 rows, 0, 100, 1 3 rows, same 6 rows (count(*) says 3) 6 rows
DataFusion 3 / 3 3 rows, correct 3 rows, correct 3 rows, correct 3 rows, correct
DuckDB (renames to dup_1) 3 / 3 3 rows, correct 3 rows, correct 3 rows, correct 3 rows, correct

Two things follow.

Comet reads a duplicate-named struct correctly when the file is well formed. On the DataFusion-written file — same duplicate dup/dup struct, same spark.read.schema("s struct<dup: bigint>") — Spark and Comet both return [0], [1], [2] with CometNativeScanExec in the plan. DuckDB refuses to construct the shape at all (Binder Error: Duplicate struct entry name "dup").

The file Spark writes is internally inconsistent: the row group declares 3 rows while each of the two same-path s.dup leaf chunks declares 6 values. parquet-mr cannot read it (ParquetEncodingException: [s, dup] -(0)-> 2). Spark's own 3 rows are 0, 100, 1, the two children interleaved — the right row count over the wrong values. So there is no correct answer to match on that file, and #5783's premise that Spark is right and Comet is wrong does not hold.

Practical consequence for #5786: rejecting duplicate Parquet field names before decoding would make Comet refuse the DataFusion-written file, which it currently reads correctly and in agreement with Spark. If a guard is wanted there, keying it on the num_values vs row_group_num_rows inconsistency rather than on duplicate names would avoid that regression. cc @dwsmith1983

Separately, DataFusion answers count(*) from row group metadata and the scan from num_values, so on the malformed file it returns 3 and 6 in the same session. Filed upstream as apache/datafusion#25433.

@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 7fe93280 against 23a9ff64. The root-field-ID P2 is fixed: the schema entry point now checks root IDs, and the flattened metadata-free Parquet fixture covers the earlier failure. The suite-registration P2 remains resolved after removing the duplicate suite and moving its unique cases into the registered scan-rule suite.

The shallow root and nested checks avoid repeated subtree traversal. The shared name predicate preserves shuffle behavior, and the metadata helper now uses Spark's field-ID APIs. No remaining P1/P2 findings.

Validation: the fresh component probe passed 20 schema controls and seven helper checks, including the original root case, flag-off behavior and unambiguous projections. Local suite-registration checks pass. On merge 3a9184d8, the scan job passed both root and nested reader fixtures, and the execution job passed the scan-rule and cache regressions. All seven changed source/test files match the reviewed head.

Local validation was a component probe with explicit stubs, not a full Spark/JNI run. Maintained Spark 3.5/4.0 semantics were checked. Local maintained 3.4/4.1 sources were unavailable.

@comphead
comphead added this pull request to the merge queue Sep 18, 2026
Merged via the queue into apache:main with commit 1adf5e3 Sep 18, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading area:shuffle Shuffle (JVM and native) bug Something isn't working run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate field ids inside a struct are not validated when the file schema equals the requested schema and no predicate is pushed

3 participants