Skip to content

test: cover container field ids next to INT96 timestamps in the native Parquet scan - #6862

Merged
sunchao merged 1 commit into
apache:mainfrom
dwsmith1983:test/6131-int96-container-field-ids
Oct 11, 2026
Merged

sunchao merged 1 commit into
apache:mainfrom
dwsmith1983:test/6131-int96-container-field-ids

Conversation

@dwsmith1983

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #6131.

Rationale for this change

With spark.sql.parquet.fieldId.read.enabled=true, the native scan matches fields by id on the Arrow schema it gets after DataFusion's INT96 coercion. That coercion rebuilt struct, list and map fields without their metadata, so an id that lived only on a container was lost and the column came back as nulls while Spark read the data. Spark writes timestamps as INT96 by default, so any id-bearing container with a timestamp under it was affected.

The DataFusion 55.2.0 upgrade in #6766 resolved this. It carries apache/datafusion#25895 (the branch-55 backport of apache/datafusion#24790), which keeps field metadata when coercing INT96 timestamps. Comet needs no code change, so this PR adds tests that keep the case covered.

What changes are included in this PR?

Tests in ParquetReadSuite. Each writes INT96 timestamps with Spark, then reads with field ids on and the id-bearing field renamed, so the id is the only way to match it:

  • an id on a struct, list or map column holding a timestamp
  • an id on a struct nested below a struct and below a list, with an id on its timestamp leaf as well
  • an id on a Variant column next to a timestamp (Spark 4.0+)

How are these changes tested?

The new tests fail on the commit before #6766 (a56db5a6c), where Comet returns nulls for each renamed column, and pass on main with Spark 3.4, 3.5, 4.0 and 4.1. For the nested case, Spark's own vectorized reader before 4.0 rejects the renamed struct below the list, so that test compares with Spark from 4.0 on and checks the native scan and the expected rows on every version.

…e Parquet scan

DataFusion 55.2.0 keeps field metadata when coercing INT96 timestamps, so a field id on a struct, list, map or Variant column now survives into the native scan's id matching. Add tests for root and nested containers that read the column under a new name, so only the id can match it.
@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Oct 11, 2026

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

Reviewed full head ae76e2314c3c2e0c64efec0e28bf728005895b30 against base 78b0eb0df342e631a619c4951fd9b531f0375a3e. The entire diff is 110 added lines in ParquetReadSuite.scala. The PR is not a draft. Snapshot and live discussion checks found no existing reviews, comments, or threads.

Routed skills: .ai/skills/review-comet-pr/SKILL.md, supplemented by the contributor timezone guidance. No sibling review skill applies to these test-only changes.

Summary

  • Prior state and problem: DataFusion’s INT96 coercion previously dropped container metadata, making Comet return nulls when matching container fields by ID. The DataFusion 55.2.0 upgrade already fixed this before the supplied base. This PR adds regression coverage.
  • Design approach: Write Spark-generated INT96 files, retain field IDs while renaming requested fields, and verify that native reads recover the data. Cases cover structs, lists, maps, nested containers, and Variant beside a timestamp.
  • Correctness: Renaming removes accidental name matching. Spark comparisons, pinned rows, and native-plan assertions check results and execution. Spark’s clipParquetGroupFields and DataFusion’s metadata-preserving coercion support the expected behavior. No introduced P1/P2 correctness issue was identified.
  • Compatibility analysis: Checked relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. The nested comparison guard matches the converter’s field-ID fallback added in 4.0. Utils.variantType avoids unsupported Variant usage on 3.x. No compatibility regression was identified.
  • Key design decisions: Explicitly disabling Comet during writes and selecting INT96 isolate the reader regression. Renamed container and timestamp fields test ID resolution at multiple levels. Null containers and nested nulls help distinguish successful reads from erroneous null filling.
  • Implementation sketch: Three parameterized container tests share a small timestamp fixture. Separate tests exercise nested renaming and Variant metadata preservation. Existing ParquetReadV1Suite registration covers the additions in Linux and macOS workflows.
  • Performance: Production execution is unchanged. Added work consists of five small file-based tests and their assertions. No evidence-backed P1/P2 performance concern was identified.
  • Design: Keeping this coverage in the existing Parquet suite directly exercises the integration that exposed the bug. No additional production mechanism is needed.
  • Abstraction & complexity: The small case table and local schema builder avoid repetition without introducing a new testing framework. No complexity issue meeting the reporting bar was identified.
  • Behavioral changes worth calling out: This PR changes test coverage only. Compared with branch-1.1, which uses DataFusion 55.1.0, the covered read improvement comes from the already-landed dependency upgrade, not this diff.
  • Suggested improvements: No code improvement meeting the P1/P2 reporting bar was identified. Functional CI validation remains outstanding as described below.

Exact-head CI: Comet CI, PR title checking, and CodeQL report action_required. Comet CI has zero jobs. Only the labeling workflow passed, so there is no functional CI verdict for this head.

Validation: A bounded Spark 3.5.9 reproduction passed the struct, list, and map fixtures, including null rows, and verified INT96 and field IDs in the Parquet footers. It reproduced the documented nested vectorized-reader failure. git diff --check passed. The checkout remained unchanged.

Validation limits: The Comet Scala/native suite was not built or executed locally because this checkout has no compiled artifacts. Other Spark versions were checked against source. The author-reported cross-version native results were not independently reproduced.

No introduced P1/P2 issues found within this review.

@sunchao
sunchao added this pull request to the merge queue Oct 11, 2026
@sunchao

sunchao commented Oct 11, 2026

Copy link
Copy Markdown
Member

Merged, thanks @dwsmith1983 !

Merged via the queue into apache:main with commit d735166 Oct 11, 2026
41 checks passed
@dwsmith1983
dwsmith1983 deleted the test/6131-int96-container-field-ids branch October 11, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Field id matching misses a container id after INT96 coercion drops container metadata

2 participants