mbutrovich opened a new issue, #23978:
URL: https://github.com/apache/datafusion/issues/23978
### Describe the bug
apache/datafusion#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`:
```rust
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
- apache/datafusion#22795: the original design doc for the skip optimization.
- apache/datafusion#22857 / branch-54 backport #23088: introduced the
reordering and the bypass.
- apache/datafusion-comet#3978: where this was diagnosed downstream. Comet's
workaround (forcing eager, always-cached page-index loading via a Comet-owned
`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.
- Relevant code: `load_page_index` (module-level free function) and
`RowGroupsPrunedParquetOpen::load_page_index` in
`datafusion/datasource-parquet/src/opener/mod.rs`;
`DFParquetMetadata::fetch_metadata` / `CachedParquetFileReaderFactory` in
`datafusion/datasource-parquet/src/metadata.rs` and `reader.rs`.
--
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]
