dwsmith1983 commented on code in PR #6116:
URL: https://github.com/apache/datafusion-comet/pull/6116#discussion_r4078294146
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -852,6 +849,21 @@ impl PhysicalExprAdapterFactory for
SparkPhysicalExprAdapterFactory {
// to the original physical names. This is necessary because
downstream code
// (reassign_expr_columns) looks up columns by name in the actual
stream schema,
// which uses the original physical file column names.
+ //
+ // Before any of that, mirror the eager check in Spark's
`ParquetReadSupport`: a read
+ // schema that carries field ids at any depth may not read a file that
carries none at
+ // any depth, unless `ignoreMissing` is set. Spark applies this check
whether or not
+ // `fieldId.read.enabled` is on, so it runs before the id matching
gate below and does
+ // not depend on the remap.
+ if !self.parquet_options.ignore_missing_field_id
Review Comment:
> Would it work to compute the logical half at plan time from
`required_schema`, which `init_datasource_exec` already has in scope?
Yes, and your reading of the schemas is right: `init_datasource_exec` builds
DataFusion's table schema from `data_schema` when the projection resolves by
name, so `logical_file_schema` in `create` was the full read schema rather than
the pruned one Spark checks. ba0d8d7c2 adds `requested_schema_has_field_ids` to
`SparkParquetOptions`, set once in `init_datasource_exec` from
`required_schema`. Your reproducer is pinned in Rust through the real planner
entry point and in `ParquetReadSuite`, under both read flag settings: the
pruned read returns the two rows on the native scan and the unpruned read still
raises.
The file half moved as well, for a reason that turned up while checking edge
cases. DataFusion's INT96 coercion rebuilds struct, list and map containers
without their metadata, so a file whose only id sits on a struct that holds a
timestamp column lost that id in `physical_file_schema`, and the check raised
where Spark's `containsFieldIds` over the raw message type passes. Ids on a
repeated list or key value group never reach the Arrow schema at all. So the
check now runs in the eager page index reader's `get_metadata`, walking the raw
Parquet schema from the footer, and the JNI error conversion unwraps the error
so the Java side still sees the same exception. Tests cover the timestamp
struct, an id only on the list group, a directory mixing files with and without
ids, nested schema pruning, id zero, `count(*)` and the exception class on 3.x
and 4.x. One related, pre-existing divergence stays out of this change and is
tracked in #6131: with id matching on, the remap sees the coerc
ed schema too, so a container that lost its id is null filled where Spark
matches the raw group id.
--
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]
