Conversation
…atching is on (apache#6116) * fix: reject a file without field ids at any depth whether or not id matching is on Spark's `ParquetReadSupport.getRequestedSchema` raises when the requested schema carries a Parquet field id at any depth and the file carries none at any depth, unless `spark.sql.parquet.fieldId.read.ignoreMissing` is set. The check runs on every read, for both readers, and does not consult `spark.sql.parquet.fieldId.read.enabled`. The native scan ran that check only with the read flag on, only over root fields, and only inside the name remap, which a case-sensitive session with the flag off never reaches. A read schema with ids over a file without ids returned rows where Spark raises, and a file whose ids sit only on nested fields was rejected where Spark reads it and null-fills the unmatched fields. The check now runs at the top of the expression adapter factory with a recursive predicate on both schemas. Id matching itself stays gated on the read flag and root ids, as Spark gates it per struct level. The Iceberg scan opts out, since its reader resolves columns by id and supplies the ids itself. * fix: answer both halves of the missing-field-id check from what Spark reads Spark checks the pruned required schema for ids and the raw Parquet schema for their absence. The requested half now comes from `required_schema` at plan time, since the logical schema DataFusion hands the adapter is the full read schema, so an id on a column the query never projects no longer raises. The file half moves into the eager page index reader, which walks the raw schema from the footer. The Arrow schema the adapter sees has lost container metadata after INT96 coercion and never carries ids on repeated list or key value groups, so a file whose only id sat on a struct holding a timestamp raised where Spark reads. The JNI error conversion unwraps the Parquet external error so the Java side keeps the same exception, and the Iceberg scan no longer needs an opt-out since it does not use that reader. Tests cover the pruned projection, the timestamp struct, an id only on a list or key value group, a directory mixing files with and without ids, nested schema pruning, id zero, count(*), and the exception class on each version. * test: find the missing field ids error anywhere in the cause chain On Spark 4.0 and 4.1 the RuntimeException Spark raises for a read schema with field ids over a file without any arrives wrapped once, in the FAILED_READ_FILE SparkException that collect raises, and the helper assumed one more layer above it. It now walks the cause chain and requires a plain RuntimeException carrying Spark's message, for Spark and for Comet alike, so the layer count no longer matters. * fix: let the serde say whether the file must carry field ids The requested side of the missing field ids check is Spark's own hasFieldIds over the required schema, so the serde now sends one flag, require_field_ids, in place of ignore_missing_field_id, and the native Arrow walk is gone. The reader factory takes that one flag through a builder and the parquet options carry nothing for it. The error names the file, and the module doc leads with the lasting reason the check reads the raw footer: ids on repeated groups and on the message root never reach Arrow. The Rust scan tests that had Scala twins are gone, the repeated group cases moved to Scala where they compare against Spark, and the Scala tests fold into the existing port and one pruned schema test with Spark's own assertion form. (cherry picked from commit 863f11d)
andygrove
added
bug
sunchao
approved these changes
Sep 27, 2026
sunchao
left a comment
sunchao
left a comment
Member
There was a problem hiding this comment.
Summary
- Prior state and problem: Native Parquet scans checked missing field IDs only during ID matching and only at the root, causing both incorrect acceptance and rejection of files.
- Design approach: Validate ID presence against the raw Parquet footer, independently of ID-based column matching.
- Correctness / compatibility analysis: The validation rule matches Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Reviewed schema pruning, nested IDs, metadata-cache reuse, mixed files, and exception conversion.
- Key design decisions: Reusing the existing metadata hook preserves IDs lost during Arrow conversion. The gated, short-circuit schema traversal adds no file reads or per-batch work.
- Implementation sketch: Scala serializes
require_field_ids, the reader checkscontains_field_ids, and JNI preserves the structuredParquetMissingFieldIdserror through Parquet wrappers. - Behavioral changes worth calling out: Files without IDs are rejected even when ID matching is disabled, unless
ignoreMissingis enabled. IDs on nested fields and repeated container groups count. Unread ID-bearing fields do not require IDs. - Suggested improvements: No introduced P1/P2 issues found within this review.
Reviewed all 10 changed files at 94641069685f1bc044586ccc0c36cf402a960d23 against 499ee08e1b0b47ebf17ef29ed5b7b0eb91906741. The PR is not a draft. Snapshot and live discussion checks found no reviews, comments, or threads. Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-ffi-pr.
Exact-head CI: Verified the tested PR merge commit has the same tree as the requested head. Rust tests passed (1,724), including all three new tests. The scans job passed (597 tests), including all six changed or added Parquet tests. Spark SQL core label-run shards passed, including ParquetFieldIdIOSuite. Four SQL jobs remain running. An earlier label-run preflight and its aggregate check failed because Maven download returned HTTP 403 before building.
Validation limits: The focused JNI error-unwrapping test passed locally. The local core metadata test could not build because hdfs-sys could not find jni.h. Scala tests were verified through CI, not rerun locally. Other Spark versions received source comparison, not runtime testing. No performance benchmark was run. The project working tree remains unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #6116 to
branch-1.1.Cherry-picked from
863f11de5eb78bacf11737032cdbcf496ad520b5without conflicts. The ten files it changes are identical onbranch-1.1and onmainjust before #6116, so the diff is byte-identical to upstream.Which issue does this PR close?
Closes #5936 on
branch-1.1. #6116 already closed it onmain.Rationale for this change
#6116 was on the list of fixes to merge before cutting 1.1.0 in #5327, but it merged after
branch-1.1was cut at 36ab57c. Without it, 1.1.0 ships the divergence from Spark described in #5936. The native scan checks for missing field ids only whenspark.sql.parquet.fieldId.read.enabledis on, and only over root fields. So a read schema with ids over a file without ids returns rows where Spark raises, and a file whose ids sit only on nested fields is rejected where Spark reads it.What changes are included in this PR?
The fix is the original one, so see #6116 for the details. No adaptations were needed.
How are these changes tested?
The original PR's tests, run locally on
branch-1.1with the default profile (Spark 4.1, Scala 2.13, JDK 17):eager_page_index_reader_factory.rs,contains_field_ids_sees_ids_on_any_nodeandget_metadata_refuses_a_file_without_ids_only_when_required, pass along with the rest of the core crate'sparquetmodule, 226 tests in all. The newparquet_external_spark_error_keeps_its_typepasses with the rest ofdatafusion-comet-jni-bridge(29 tests), anddatafusion-comet-commonpasses (58 tests).ParquetReadV1Suitethat fix: reject a file without field ids at any depth whether or not id matching is on #6116 added or changed pass, as does the existingnested field ids resolve by id below struct, list and map, not by position.cargo fmt --all -- --checkandcargo clippy --all-targets --workspace -- -D warningspass on rustc 1.98.1.Spark's own
ParquetFieldIdIOSuitecovers this path in the Spark SQL job, which a pull request againstbranch-1.1runs only when labeled, so this carriesrun-spark-4.1-testsas #6116 did.