mbutrovich opened a new pull request, #5132: URL: https://github.com/apache/datafusion-comet/pull/5132
## Which issue does this PR close? Part of #3978. ## Rationale for this change Comet's `native_datafusion` Parquet scan regressed on q88-shaped queries (three `IS NOT NULL` predicates on non-null foreign keys against a wide fact table, scanned across thousands of files and partitions) between DataFusion 54.0.0 and 54.1.0. Duration on one benchmark went from 24s to 34s for that query alone. The cause is upstream, in `datafusion-datasource-parquet`'s opener. apache/datafusion#22857 (backported to branch-54 as #23088, first released in 54.1.0) reordered the opener so the initial metadata load requests `PageIndexPolicy::Skip`, and defers loading the page index until row-group statistics can't already prove the surviving row groups are fully matched. That's a real win when the skip fires. When it doesn't, the deferred load (`load_page_index` in `opener/mod.rs`) reads the page index directly off the `AsyncFileReader`'s byte-range methods, bypassing `ParquetFileReaderFactory::get_metadata` and therefore the `FileMetadataCache` entirely. The result is never written back to the cache the initial load populated. `store_sales`'s row groups don't carry the `null_count` statistics the skip needs for `IS NOT NULL`, so the skip never fires for this query, and every open of the same file now pays for a fresh, uncached page-index fetch, once per row-group split. Comparing 54.0.0 to 54.1.0 on the same query and data, cumulative `CometNativeScan` metrics for `store_sales` moved: | metric (summed across the scan's task instances) | 54.0.0 | 54.1.0 | | --- | --- | --- | | Wall clock time elapsed for file opening | 79.5 min | 149.8 min (+88%) | | Wall clock time elapsed for data decompression + decoding | 57.0 min | 77.6 min (+36%) | | Total time reading and parsing footer metadata | 79.0 min | 89.1 min (+13%) | | Number of bytes scanned | 3.4 GiB | 3.5 GiB (+3%) | Bytes scanned barely moved; file-opening wall clock nearly doubled. That's a request-count/latency signature, not a data-volume one, consistent with a second, uncached fetch being added per open rather than more data being read. This isn't specific to DataFusion 54.1: it's a gap in how `load_page_index` in the opener interacts with `FileMetadataCache`, present in current DataFusion main too. Filed upstream as apache/datafusion#23978. This PR is a Comet-side workaround, not a fix for the upstream gap. ## What changes are included in this PR? - 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. Once the page index is present in that first load, the opener's own `missing_column_index || missing_offset_index` guard in `load_page_index` sees it's already there and 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: this gives up the benefit of #22857's skip for files where it would have legitimately avoided the page index load (selective predicates, row groups with real `null_count` stats). Given `store_sales` never benefits from the skip here regardless of DataFusion version, and the alternative is unbounded repeated I/O at scale, that trade is worth it for now. Revert this once apache/datafusion#23978 is fixed. ## How are these changes tested? Updated `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. No new integration/benchmark coverage in this PR; the regression itself was found via TPC-DS q88 wall-clock comparison across DataFusion versions, not a unit test, and isn't something existing CI would catch (bytes-scanned and duration deltas at this scale need the full SF1000-class dataset). -- 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]
