andygrove opened a new pull request, #5153:
URL: https://github.com/apache/datafusion-comet/pull/5153
## Which issue does this PR close?
Backport of #5107 (merged to `main` as `0b2a50608`) onto `branch-1.0`.
Closes #4990 for the 1.0.0 release branch.
## Rationale for this change
Comet's native Parquet scan built a fresh `TableParquetOptions::new()` per
scan and never looked at the session's `datafusion.execution.parquet.*` config.
`pushdown_filters` appeared to work only because
`ParquetSource::try_pushdown_filters` happens to OR the session value in
independently; every other reader option (`reorder_filters`,
`force_filter_selections`, `bloom_filter_on_read`, `max_predicate_cache_size`,
`enable_page_index`, `pruning`) was silently inert, including when driven by
`spark.comet.parquet.rowFilterPushdown.enabled`.
That means `spark.comet.parquet.rowFilterPushdown.enabled` — a user-facing
config on `branch-1.0` — does not actually enable row-level pushdown at the
native scan, and the `spark.comet.datafusion.execution.parquet.*` escape hatch
(behind `spark.comet.exec.respectDataFusionConfigs`) is inert for every reader
option. Worth carrying into 1.0.0 rather than shipping configs that silently do
nothing.
Fixing this also exposed a precedence bug: the `rowFilterPushdown.enabled`
flag unconditionally overwrote the raw
`spark.comet.datafusion.execution.parquet.{pushdown_filters,reorder_filters}`
escape hatch, even when a developer set those keys explicitly for testing.
## What changes are included in this PR?
A clean `git cherry-pick -x` of `0b2a50608`. No conflicts and no adaptation
was needed — `git diff` against the commit on `main` is byte-identical.
- `native/core/src/parquet/parquet_exec.rs`: `get_options()` now takes the
session's `ParquetOptions` and seeds `table_parquet_options.global` for the
fields confirmed to reach `ParquetSource`'s actual per-file execution path
(`pushdown_filters`, `reorder_filters`, `force_filter_selections`,
`bloom_filter_on_read`, `max_predicate_cache_size`, `enable_page_index`,
`pruning`). Deliberately an allowlist rather than a wholesale `global =
session.clone()`, so a new DataFusion reader option is ignored until someone
checks it is safe for Spark semantics. `coerce_int96`/`coerce_int96_tz` stay
hardcoded afterward, non-overridable, for Spark INT96 compatibility.
- Not wired up: `schema_force_view_types`, `binary_as_string`,
`skip_metadata`, `global.metadata_size_hint`. All four are only consumed by
`ParquetFormat::infer_schema`, which Comet's scan construction never calls
(Comet always supplies its own schema translated from Spark's catalog), so
seeding them would be a no-op.
- `init_datasource_exec` now computes `session_ctx.copied_config()` once and
reuses it for both `get_options()` and `try_pushdown_filters`, instead of
calling `session_ctx.state()` — which clones the whole `SessionState`,
including the function registries and optimizer rules — on every scan init.
- `native/core/src/execution/jni_api.rs`: the `rowFilterPushdown.enabled`
translation moves to *before* the `spark.comet.datafusion.*` pass-through loop,
so an explicit override of `pushdown_filters`/`reorder_filters` wins instead of
being silently forced back to `true`.
## How are these changes tested?
By the tests added in #5107, verified on this branch:
- `cargo test -p datafusion-comet --lib parquet_exec` — 2 passed, including
the new
`seeds_allowlisted_fields_from_session_but_protects_spark_compat_fields`, which
asserts `get_options()` carries the allowlisted fields through from the session
while keeping `coerce_int96`/`coerce_int96_tz` hardcoded.
- `CometNativeReaderSuite` under `-Pspark-3.5` — 54 passed, 2 canceled
(pre-existing version-gated cancellations). Includes the three new tests:
row-level pushdown only fires at the native scan once
`rowFilterPushdown.enabled` is set (the core #4990 regression), an explicit
`pushdown_filters=false` override wins over that flag, and an explicit
`pruning=false` override actually disables row-group statistics pruning.
- `cargo clippy --all-targets --workspace -- -D warnings` — clean.
JDK 11 locally, hence `-Pspark-3.5`; the Spark 4.x lanes are left to CI.
--
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]
