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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
