andygrove opened a new pull request, #5142: URL: https://github.com/apache/datafusion-comet/pull/5142
## Which issue does this PR close? Backport of #5132 to `branch-1.0`. Part of #3978. ## Rationale for this change `branch-1.0` is on DataFusion 54.1.0, so it carries the same `native_datafusion` Parquet scan regression that #5132 works around on `main`. Upstream apache/datafusion#22857 (backported to branch-54 as #23088, first released in 54.1.0) reordered the Parquet opener so the initial metadata load requests `PageIndexPolicy::Skip`, deferring the page-index load until row-group statistics can't already prove the surviving row groups are fully matched. When that skip doesn't fire, the deferred `load_page_index` reads the page index directly off the `AsyncFileReader`'s byte-range methods, bypassing `ParquetFileReaderFactory::get_metadata` and therefore the `FileMetadataCache`, and the result is never written back to the cache the initial load populated. For q88-shaped queries (three `IS NOT NULL` predicates on non-null foreign keys against a wide fact table), `store_sales`'s row groups don't carry the `null_count` statistics the skip needs, so every open of the same file pays a fresh, uncached page-index fetch, once per row-group split. On the original benchmark, that query went from 24s to 34s between 54.0.0 and 54.1.0, with file-opening wall clock nearly doubling while bytes scanned moved only 3%. Tracked upstream as apache/datafusion#23978. This is a Comet-side workaround, not a fix for the upstream gap, and should be reverted once that is fixed. ## What changes are included in this PR? Clean cherry-pick of cde6052 with no conflicts and no adaptation needed: - Add `EagerPageIndexReaderFactory` (`native/core/src/parquet/eager_page_index_reader_factory.rs`), a `ParquetFileReaderFactory` that ignores the `PageIndexPolicy` DataFusion's opener requests and always fetches the page index eagerly via `DFParquetMetadata::with_page_index_policy(Some(PageIndexPolicy::Optional))`, caching it in the same shared `FileMetadataCache` DataFusion's own factory uses. With the page index present after that first load, the opener's `missing_column_index || missing_offset_index` guard in `load_page_index` never issues the uncached fetch. - Wire it into `init_datasource_exec` in `native/core/src/parquet/parquet_exec.rs`, replacing `CachedParquetFileReaderFactory`. - Update the existing regression test (`caches_full_metadata_with_page_index`, added by #4707) to assert the page index is present in the cache even for a scan with no pruning predicate, since this factory no longer depends on a predicate driving the load. The tradeoff is the same as on `main`: this gives up #22857's skip for files where it would have legitimately avoided the page-index load (selective predicates, row groups with real `null_count` stats), in exchange for avoiding unbounded repeated I/O at scale. ## How are these changes tested? `caches_full_metadata_with_page_index` in `parquet_exec.rs` writes a Parquet file with a page index, runs a scan through `init_datasource_exec` with no pruning predicate, and asserts the `RuntimeEnv` metadata cache holds the column and offset index after the first open. Verified locally on this branch alongside `cargo clippy -p datafusion-comet --all-targets -- -D warnings`. -- 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]
