You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "dwsmith1983 (via GitHub)" <gi...@apache.org> on 2026/09/30 06:59:07 UTC
[PR] fix: fall back to Spark for Parquet field ids on nested fields [datafusion-comet]
dwsmith1983 opened a new pull request, #6444:
URL: https://github.com/apache/datafusion-comet/pull/6444
## 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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for Parquet field ids on nested fields [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6444:
URL: https://github.com/apache/datafusion-comet/pull/6444#issuecomment-5981399071
@dwsmith1983 https://github.com/apache/datafusion/pull/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for Parquet field ids on nested fields [datafusion-comet]
Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.
dwsmith1983 commented on PR #6444:
URL: https://github.com/apache/datafusion-comet/pull/6444#issuecomment-5907811661
@andygrove could you approve a CI run on `e17557457`? This is the stopgap for #6131 discussed on #6405.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for Parquet field ids on nested fields [datafusion-comet]
Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.
dwsmith1983 commented on PR #6444:
URL: https://github.com/apache/datafusion-comet/pull/6444#issuecomment-5981443849
> @dwsmith1983 [apache/datafusion#25895](https://github.com/apache/datafusion/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for Parquet field ids on nested fields [datafusion-comet]
Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.
dwsmith1983 closed pull request #6444: fix: fall back to Spark for Parquet field ids on nested fields
URL: https://github.com/apache/datafusion-comet/pull/6444
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org