Repository navigation
fix: fall back to Spark for Parquet field ids on nested fields - #6444
dwsmith1983 wants to merge 8 commits into
Conversation
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.
|
@andygrove could you approve a CI run on |
sunchao
left a comment
There was a problem hiding this comment.
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:
hasContainerFieldIdsrecursively checks metadata,CometScanRulepreserves 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 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 |
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. |
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?
CometScanRulefalls 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.hasContainerFieldIdswalks the schema and flags an id on a struct, array, map or Variant field.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.How are these changes tested?
ParquetReadSuitewrite 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.CometScanRuleSuitecovers the schema walk: struct, array, map, nested and Variant positions, and schemas that must not match.ParquetReadV1SuiteandCometScanRuleSuitepass on Spark 3.5 and 4.1, and the new native test passes against DataFusion 55.1.