Skip to content

fix: fall back to Spark for Parquet field ids on nested fields - #6444

Closed
dwsmith1983 wants to merge 8 commits into
apache:mainfrom
dwsmith1983:fix/6131-container-id-fallback
Closed

dwsmith1983 wants to merge 8 commits into
apache:mainfrom
dwsmith1983:fix/6131-container-id-fallback

Conversation

@dwsmith1983

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #6131. The issue stays open to track removing this fallback once Comet picks up a DataFusion release with apache/datafusion#24790 (backport to branch-55 in apache/datafusion#25895).

Rationale for this change

With spark.sql.parquet.fieldId.read.enabled=true, the native scan returns nulls for a struct, array or map column whose field carries a Parquet field id when the file also holds an INT96 column. DataFusion 55.1 rebuilds every struct, list and map field without its metadata when it coerces INT96 timestamps, so the file side loses the id and the column no longer matches the requested field. A Variant is read as a struct in the file, so an id on a Variant field (Spark 4.x) is lost the same way. Spark reads these columns correctly, so this is a wrong result rather than an error.

The planner cannot see whether a file holds INT96, so until the DataFusion fix lands the safe option is to send any such read to Spark.

What changes are included in this PR?

  • CometScanRule falls back to Spark when field id reads are on and the required schema has an id on a struct, array, map or Variant field at any depth. Ids on leaf fields, including leaves nested inside those types, still resolve natively. The code carries a TODO pointing at Field id matching misses a container id after INT96 coercion drops container metadata #6131.
  • DataTypeSupport.hasContainerFieldIds walks the schema and flags an id on a struct, array, map or Variant field.
  • A native unit test, int96_coercion_drops_container_field_ids, runs DataFusion's INT96 coercion the way the scan configures it and asserts that container ids are dropped while leaf ids survive. It fails once Comet moves to a DataFusion with the fix, and its comment names the fallback to remove and the four tests whose container ids should go back.
  • The scans compatibility page lists the new fallback and says it is temporary.

How are these changes tested?

  • New tests in ParquetReadSuite write a struct, an array and a map field with an id next to an INT96 timestamp and assert the fallback reason and the rows. Without the fix Comet returned [null] where Spark returned the data. A Spark 4.x test covers a Variant field with an id and fails the same way without the fix. Another test pins that a container id still falls back when the file has no INT96 column, and one checks that ids only on leaf fields keep the scan native.
  • Four existing tests that read containers by id were adjusted. Where a test was about leaf ids, the ids moved off the containers so it still runs natively. "read nested types by Parquet field id when names differ" now asserts the fallback for container ids and adds a native read with renamed leaves resolved by id. "ids on repeated list and key_value groups count as file ids" still runs natively with both flag values.
  • A unit test in CometScanRuleSuite covers the schema walk: struct, array, map, nested and Variant positions, and schemas that must not match.
  • ParquetReadV1Suite and CometScanRuleSuite pass on Spark 3.5 and 4.1, and the new native test passes against DataFusion 55.1.

DataFusion 55.1 rebuilds struct, list and map fields without their
metadata when it coerces INT96 timestamps, so a field id on one of those
fields (or on a Variant, read as a struct) is lost and the native scan
returns nulls where Spark reads the data. When field id reads are on and
the required schema has an id on such a field, send the scan to Spark
until Comet moves to a DataFusion release with apache/datafusion#24790.
The planner cannot see whether a file holds INT96, so the check does not
depend on it.
…ercion

Pin today's behaviour, where DataFusion 55.1 drops the field id on
struct, list and map fields when it coerces INT96, so moving to a
release with apache/datafusion#24790 fails this test and points at the
fallback to remove. The container check now decides Variant itself, so
the unit test covers the same predicate the planner uses.
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Sep 30, 2026
@dwsmith1983

Copy link
Copy Markdown
Contributor Author

@andygrove could you approve a CI run on e17557457? This is the stopgap for #6131 discussed on #6405.

@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

  • Prior state and problem: DataFusion’s INT96 coercion drops container field IDs, causing native Parquet reads to return nulls where Spark returns data.
  • Design approach: Fall back to Spark when field-ID reads are enabled and the required schema contains an ID-bearing struct, array, map, or Variant field.
  • Correctness / compatibility analysis: Checked Spark’s field-ID handling across 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Focused helper checks and Spark reference reads passed on 3.5.9 and 4.1.3, including Variant on 4.1.3. No introduced P1/P2 issues found within this review.
  • Key design decisions: The conservative guard avoids inspecting file footers during planning. It adds a short-circuiting schema walk without per-row work and reuses existing Variant shims.
  • Implementation sketch: hasContainerFieldIds recursively checks metadata, CometScanRule preserves the Spark scan with a fallback reason, and Scala tests, a native upgrade sentinel, and compatibility documentation cover the change.
  • Behavioral changes worth calling out: Container-ID reads fall back even when files contain no INT96 columns. Leaf-only IDs remain eligible for native execution.
  • Suggested improvements: None supported by reproducible P1/P2 evidence.

Reviewed the full six-file diff at 8ed70d52b69413be8c44e48977e38de9f379d4ec against 11a27c36713779f8fd297e0dd72ac4911dd3e04a. Confirmed the PR is not a draft and read the supplied discussion snapshot. Routed skill: review-comet-pr. No sibling skill applies to this scan-planning-only production change.

Exact-head CI: Comet CI and CodeQL report action_required. Only the labeling job passed, so no build/test CI verdict is available.

Validation limits: Local checks compiled the exact helper and version shims with a minimal Utils facade and exercised Spark reference reads, not the full Comet planner. The native unit-test attempt reached a 180-second dependency-build limit before test execution. Full Comet suites and Spark SQL matrix tests were not run locally.

@dwsmith1983
dwsmith1983 requested a review from sunchao October 4, 2026 15:03
@andygrove

Copy link
Copy Markdown
Member

@dwsmith1983 apache/datafusion#25895 has been merged. Should we just wait for the DF 55 patch release? It will be a few weeks before we release 1.2.0

@dwsmith1983

Copy link
Copy Markdown
Contributor Author

@dwsmith1983 apache/datafusion#25895 has been merged. Should we just wait for the DF 55 patch release? It will be a few weeks before we release 1.2.0

@andygrove

Yes, the 55.2.0 RC should be out this weekend. Once it's published I'll open a PR that bumps to 55.2.0 and carries over the INT96 + container id tests from here as native regression tests. Closing this one.

1.1.0 is on 55.1.0, so it still has the bug. Might be worth listing #6131 as a known issue in the release notes.

@dwsmith1983 dwsmith1983 closed this Oct 4, 2026
@dwsmith1983
dwsmith1983 deleted the fix/6131-container-id-fallback branch October 11, 2026 04:51
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 bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants