mbutrovich commented on code in PR #5107:
URL: https://github.com/apache/datafusion-comet/pull/5107#discussion_r3685072380
##########
native/core/src/parquet/parquet_exec.rs:
##########
@@ -220,8 +225,30 @@ fn get_options(
allow_timestamp_ltz_to_ntz: bool,
object_store_url: &ObjectStoreUrl,
encryption_enabled: bool,
+ session_parquet_options: &ParquetOptions,
) -> (TableParquetOptions, SparkParquetOptions) {
let mut table_parquet_options = TableParquetOptions::new();
+
+ // Reader options that reach `ParquetSource`'s actual per-file execution
path
+ // (`ParquetMorselizer`, built from `table_parquet_options.global`) rather
than only
+ // `ParquetFormat::infer_schema`'s schema-inference path, which Comet
never calls (Comet
+ // always supplies its own schema, translated from Spark's catalog).
Seeded from the
+ // session so `spark.comet.datafusion.execution.parquet.*` (behind
+ // `spark.comet.exec.respectDataFusionConfigs`) and `spark.comet.parquet.
+ // rowFilterPushdown.enabled` actually take effect on the native scan
(#4990); a fresh
+ // `TableParquetOptions::new()` ignored the session entirely.
+ table_parquet_options.global.pushdown_filters =
session_parquet_options.pushdown_filters;
Review Comment:
Went through every field in `ParquetOptions` against actual usage in
datasource-parquet's `source.rs` and `opener/mod.rs`. Confirmed
`schema_force_view_types`, `binary_as_string`, `skip_metadata`, and
`global.metadata_size_hint` are only read from `file_format.rs` (the
`infer_schema` path), and everything under the writer section
(`data_pagesize_limit`, `write_batch_size`, `writer_version`,
`statistics_enabled`, `compression`, `dictionary_*`, `max_row_group_size`) is
writer-only. So `table_parquet_options.global =
session_parquet_options.clone()` would be safe for every field that exists in
`ParquetOptions` today.
I'd rather keep the explicit allowlist, though. If DataFusion adds a new
reader option to `ParquetOptions.global`, the wholesale copy makes it silently
active on every Comet scan the next time we bump the DataFusion dependency,
with no Comet-side check on whether that option is safe for Spark semantics.
`coerce_int96`/`coerce_int96_tz` are exactly this case: a reader option that
cannot be passed through as-is. I'd rather a new field come in as silently
ignored, forcing a conscious decision (and a test) before it reaches the scan,
than silently active with no gate.
I'll add a comment above the allowlist flagging that it should be checked
against `ParquetOptions` whenever we bump the DataFusion version, so it doesn't
drift by accident. Applied the rest of your comments (the reordering fix,
`copied_config()`, the page-index comment, and the `pruning=false` test).
--
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]
