Describe the bug
#22857 (backported to branch-54 as #23088, released in 54.1.0) reordered the Parquet opener's state machine so the initial metadata load requests PageIndexPolicy::Skip, and the page index is loaded later, only if row-group statistics can't already prove the surviving row groups are fully matched.
When that later load does run, it goes through the free function load_page_index in datafusion/datasource-parquet/src/opener/mod.rs:
async fn load_page_index<T: AsyncFileReader>(
reader_metadata: ArrowReaderMetadata,
input: &mut T,
options: ArrowReaderOptions,
) -> Result<ArrowReaderMetadata> {
...
let mut reader = ParquetMetaDataReader::new_with_metadata(m)
.with_page_index_policy(PageIndexPolicy::Optional);
reader.load_page_index(input).await?;
...
}
This calls ParquetMetaDataReader::load_page_index, which reads directly off the AsyncFileReader's byte-range methods. It never goes through ParquetFileReaderFactory::get_metadata / DFParquetMetadata::fetch_metadata, so the result is never written back into the FileMetadataCache that the initial (Skip-policy) metadata load populated.
For files where the skip heuristic never fires, this makes every open of the same file pay for a fresh, uncached page-index fetch, for as many times as the file is opened (once per row-group split / partition). Before #22857, CachedParquetFileReaderFactory (then documented as "always loads the entire metadata, including page index, even if not required by the current query") loaded and cached both in one request. After #22857, the equivalent case (skip doesn't fire) now costs one cached footer fetch plus one uncached page-index fetch, repeated on every open.
To Reproduce
This surfaced in Apache DataFusion Comet (apache/datafusion-comet#3978) on a TPC-DS q88-style query: three IS NOT NULL predicates on non-null foreign keys against store_sales, scanned across 10,237 files and 1,824 partitions.
Comparing DataFusion 54.0.0 (pre-#22857) to 54.1.0 (post-backport) on the same query and data, cumulative CometNativeScan metrics for store_sales moved like this (summed across all task instances in the query's physical plan):
| metric |
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 (ruling out "loading more data"), while file-opening wall clock nearly doubled: a request-count/latency signature, not a data-volume signature, consistent with a second, uncached fetch being added per open.
A minimal repro (no cluster) would be: a Parquet file with a page index, a IS NOT NULL predicate on a column whose row groups don't carry null_count statistics (or use a writer that omits it), opened twice through CachedParquetFileReaderFactory sharing one FileMetadataCache. Assert that the second open's AsyncFileReader::get_metadata does not re-issue a range read for the page index. On 54.1.0 (or current main) it does; it also does after the first open in the 54.0.0 case, but there the initial factory call already fetched and cached it, so nothing further happens.
Expected behavior
Once the opener determines the page index is needed and loads it via load_page_index, the result should be merged back into the same FileMetadataCache entry that the initial (Skip-policy) load populated, so a second open of the same file for a different row-group range gets a cache hit instead of repeating the fetch. The skip optimization from #22857 should still avoid the load entirely when it can; this is about not silently losing caching in the case where it can't.
Additional context
Describe the bug
#22857 (backported to branch-54 as #23088, released in 54.1.0) reordered the Parquet opener's state machine so the initial metadata load requests
PageIndexPolicy::Skip, and the page index is loaded later, only if row-group statistics can't already prove the surviving row groups are fully matched.When that later load does run, it goes through the free function
load_page_indexindatafusion/datasource-parquet/src/opener/mod.rs:This calls
ParquetMetaDataReader::load_page_index, which reads directly off theAsyncFileReader's byte-range methods. It never goes throughParquetFileReaderFactory::get_metadata/DFParquetMetadata::fetch_metadata, so the result is never written back into theFileMetadataCachethat the initial (Skip-policy) metadata load populated.For files where the skip heuristic never fires, this makes every open of the same file pay for a fresh, uncached page-index fetch, for as many times as the file is opened (once per row-group split / partition). Before #22857,
CachedParquetFileReaderFactory(then documented as "always loads the entire metadata, including page index, even if not required by the current query") loaded and cached both in one request. After #22857, the equivalent case (skip doesn't fire) now costs one cached footer fetch plus one uncached page-index fetch, repeated on every open.To Reproduce
This surfaced in Apache DataFusion Comet (apache/datafusion-comet#3978) on a TPC-DS q88-style query: three
IS NOT NULLpredicates on non-null foreign keys againststore_sales, scanned across 10,237 files and 1,824 partitions.Comparing DataFusion 54.0.0 (pre-#22857) to 54.1.0 (post-backport) on the same query and data, cumulative
CometNativeScanmetrics forstore_salesmoved like this (summed across all task instances in the query's physical plan):Bytes scanned barely moved (ruling out "loading more data"), while file-opening wall clock nearly doubled: a request-count/latency signature, not a data-volume signature, consistent with a second, uncached fetch being added per open.
A minimal repro (no cluster) would be: a Parquet file with a page index, a
IS NOT NULLpredicate on a column whose row groups don't carrynull_countstatistics (or use a writer that omits it), opened twice throughCachedParquetFileReaderFactorysharing oneFileMetadataCache. Assert that the second open'sAsyncFileReader::get_metadatadoes not re-issue a range read for the page index. On 54.1.0 (or current main) it does; it also does after the first open in the 54.0.0 case, but there the initial factory call already fetched and cached it, so nothing further happens.Expected behavior
Once the opener determines the page index is needed and loads it via
load_page_index, the result should be merged back into the sameFileMetadataCacheentry that the initial (Skip-policy) load populated, so a second open of the same file for a different row-group range gets a cache hit instead of repeating the fetch. The skip optimization from #22857 should still avoid the load entirely when it can; this is about not silently losing caching in the case where it can't.Additional context
ParquetFileReaderFactory, giving up the skip's benefit to restore caching) is not a fix for this issue, just a way to unblock Comet while this is open upstream.load_page_index(module-level free function) andRowGroupsPrunedParquetOpen::load_page_indexindatafusion/datasource-parquet/src/opener/mod.rs;DFParquetMetadata::fetch_metadata/CachedParquetFileReaderFactoryindatafusion/datasource-parquet/src/metadata.rsandreader.rs.