dwsmith1983 opened a new pull request, #5808: URL: https://github.com/apache/datafusion-comet/pull/5808
## Which issue does this PR close? Closes #5801. This branch is stacked on #5654 and shows its four commits until that merges; the change under review here is the last commit, `fix: validate parquet field ids when the opener skips the expression adapter`. ## Rationale for this change DataFusion's Parquet opener creates the physical expression adapter only when a predicate is pushed or the file schema differs from the requested schema. #5654 validates duplicate requested field ids inside that adapter once per file, so a file with no key-value metadata whose schema equals the requested schema, read with field id matching on and no data filter, skipped the validation and read a struct with two children sharing a requested id positionally. Spark rejects that during schema clipping. Spark-written files always carry key-value metadata that arrow-rs folds into the file schema, so they always reach the adapter; the gap is metadata-free files. ## What changes are included in this PR? - The Parquet reader factory, which fetches every file's footer regardless of what the opener decides, gains an optional field id check installed at plan time only when field id matching is on and the requested schema carries a field id at any depth, so ordinary reads pay nothing. After the footer is fetched it converts the file schema to Arrow the way the opener does and resolves the requested fields with the same mapping resolver the adapter uses, raising the same duplicate field id error. The adapter path is unchanged; this makes the two agree. - A per-file memo keyed on the cached footer's identity means a file is validated once per distinct footer, not once per batch. The check costs one schema conversion and a linear walk per file, for field id reads only. - The JNI error bridge walks the cause chain so a Spark error raised inside the reader factory keeps its Spark exception class instead of surfacing as a generic read failure. One difference from the adapter path, deliberate: this check validates the columns the read projects, as Spark's clipping does, while the adapter's root-level check runs over the full data schema it is handed. A duplicate id inside a struct the read does not project is therefore reported only on the adapter path; a test pins the projected behavior. ## How are these changes tested? Rust: five new scan tests through `DataSourceExec` on a file written without key-value metadata, asserting first that the file schema equals the requested schema so the opener skips the adapter: the duplicate id is rejected, unique ids read, the check does not run with field id matching off, and the same two cases through the planner's data schema plus projection wiring. The rejection test read the struct as `[42, 43]` before the fix. Two tests for the error chain walk in the JNI bridge. Core crate 359 tests, bridge crate 30, clippy and fmt clean. Scala: a `ParquetReadV1Suite` case writes the file with parquet-mr and no key-value metadata, asserts the footer's key-value map is empty and that the plan carries the native scan, and expects Spark's duplicate field id error; it reported no exception against the previous native library. The suite passes at 65 on Spark 3.5. -- 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]
