You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/07/29 15:28:30 UTC
[PR] fix: work around DataFusion 54.1.0 Parquet page-index regression [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: work around DataFusion 54.1.0 Parquet page-index regression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5132:
URL: https://github.com/apache/datafusion-comet/pull/5132#discussion_r3675921903
##########
native/core/src/parquet/eager_page_index_reader_factory.rs:
##########
@@ -0,0 +1,176 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! A `ParquetFileReaderFactory` that always loads the Parquet page index into the shared
+//! `FileMetadataCache` on the first metadata fetch for a file, instead of deferring to
+//! DataFusion's opener.
+//!
+//! DataFusion's opener requests `PageIndexPolicy::Skip` on the initial metadata load and defers
+//! loading the page index until row-group pruning shows it is still needed
+//! (apache/datafusion#22857). That deferred load reads the page index directly off the
+//! `AsyncFileReader` (`load_page_index` in `datafusion-datasource-parquet`'s opener), bypassing
+//! `FileMetadataCache` entirely. For a predicate that never resolves to fully-matched by
+//! row-group statistics alone, such as `IS NOT NULL` on a column whose row groups don't carry
+//! `null_count` statistics, the skip never fires, and the page index is re-fetched, uncached, on
+//! every open. At the scale of a wide fact table scanned across many partitions, that is
+//! repeated, unbounded I/O for the same bytes.
+//!
+//! This factory forces `PageIndexPolicy::Optional` on every metadata fetch, ignoring whatever
+//! policy the caller requests, so the page index is always present in the cached
+//! `ParquetMetaData` after the first load. DataFusion's opener checks whether the metadata it
+//! already has includes the page index before issuing its own fetch, so with this factory that
+//! check is always true and the uncached fetch never happens. The tradeoff: files where the
+//! opener's skip heuristic would have avoided the page index load entirely now load it anyway.
+//!
+//! Filed upstream as apache/datafusion#23978. Revert this once the opener merges its deferred
+//! page-index load back into `FileMetadataCache` instead of bypassing it.
+
+use bytes::Bytes;
+use datafusion::common::Result as DFResult;
+use datafusion::datasource::physical_plan::parquet::metadata::DFParquetMetadata;
+use datafusion::datasource::physical_plan::parquet::{
+ ParquetFileMetrics, ParquetFileReaderFactory,
+};
+use datafusion::execution::cache::cache_manager::FileMetadataCache;
+use datafusion::physical_plan::metrics::ExecutionPlanMetricsSet;
+use datafusion_datasource::PartitionedFile;
+use futures::future::BoxFuture;
+use futures::FutureExt;
+use object_store::ObjectStore;
+use parquet::arrow::arrow_reader::ArrowReaderOptions;
+use parquet::arrow::async_reader::{AsyncFileReader, ParquetObjectReader};
+use parquet::file::metadata::{PageIndexPolicy, ParquetMetaData};
+use std::fmt::Debug;
+use std::ops::Range;
+use std::sync::Arc;
+
+#[derive(Debug)]
+pub struct EagerPageIndexReaderFactory {
+ store: Arc<dyn ObjectStore>,
+ metadata_cache: Arc<dyn FileMetadataCache>,
+}
+
+impl EagerPageIndexReaderFactory {
+ pub fn new(store: Arc<dyn ObjectStore>, metadata_cache: Arc<dyn FileMetadataCache>) -> Self {
+ Self {
+ store,
+ metadata_cache,
+ }
+ }
+}
+
+impl ParquetFileReaderFactory for EagerPageIndexReaderFactory {
+ fn create_reader(
+ &self,
+ partition_index: usize,
+ partitioned_file: PartitionedFile,
+ metadata_size_hint: Option<usize>,
+ metrics: &ExecutionPlanMetricsSet,
+ ) -> DFResult<Box<dyn AsyncFileReader + Send>> {
+ let file_metrics = ParquetFileMetrics::new(
+ partition_index,
+ partitioned_file.object_meta.location.as_ref(),
+ metrics,
+ );
+ let mut inner = ParquetObjectReader::new(
+ Arc::clone(&self.store),
+ partitioned_file.object_meta.location.clone(),
+ )
+ .with_file_size(partitioned_file.object_meta.size);
+ if let Some(hint) = metadata_size_hint {
+ inner = inner.with_footer_size_hint(hint);
+ }
+
+ Ok(Box::new(EagerPageIndexReader {
+ file_metrics,
+ store: Arc::clone(&self.store),
+ inner,
+ partitioned_file,
+ metadata_cache: Arc::clone(&self.metadata_cache),
+ metadata_size_hint,
+ }))
+ }
+}
+
+struct EagerPageIndexReader {
+ file_metrics: ParquetFileMetrics,
+ store: Arc<dyn ObjectStore>,
+ inner: ParquetObjectReader,
+ partitioned_file: PartitionedFile,
+ metadata_cache: Arc<dyn FileMetadataCache>,
+ metadata_size_hint: Option<usize>,
+}
+
+impl AsyncFileReader for EagerPageIndexReader {
+ fn get_bytes(&mut self, range: Range<u64>) -> BoxFuture<'_, parquet::errors::Result<Bytes>> {
+ let bytes_scanned = range.end - range.start;
+ self.file_metrics.bytes_scanned.add(bytes_scanned as usize);
+ self.inner.get_bytes(range)
+ }
+
+ fn get_byte_ranges(
+ &mut self,
+ ranges: Vec<Range<u64>>,
+ ) -> BoxFuture<'_, parquet::errors::Result<Vec<Bytes>>>
+ where
+ Self: Send,
+ {
+ let total: u64 = ranges.iter().map(|r| r.end - r.start).sum();
+ self.file_metrics.bytes_scanned.add(total as usize);
+ self.inner.get_byte_ranges(ranges)
+ }
+
+ fn get_metadata<'a>(
+ &'a mut self,
+ _options: Option<&'a ArrowReaderOptions>,
+ ) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {
+ // Ignore the caller-requested policy: always fetch and cache the page index eagerly so
+ // the opener never has a reason to load it again, uncached, later in the same open.
+ let object_meta = self.partitioned_file.object_meta.clone();
+ let metadata_cache = Arc::clone(&self.metadata_cache);
+ let store = Arc::clone(&self.store);
+ let metadata_size_hint = self.metadata_size_hint;
+ async move {
+ DFParquetMetadata::new(store.as_ref(), &object_meta)
+ .with_file_metadata_cache(Some(metadata_cache))
+ .with_metadata_size_hint(metadata_size_hint)
+ .with_page_index_policy(Some(PageIndexPolicy::Optional))
+ .fetch_metadata()
+ .await
Review Comment:
`get_metadata` ignores `options`, which drops the file decryption properties DataFusion's opener passes in.
`CachedParquetFileReaderFactory` — the factory this replaces — forwards them (`datafusion-datasource-parquet-54.1.0/src/reader.rs:291`):
```rust
let file_decryption_properties = options
.and_then(|o| o.file_decryption_properties())
.map(Arc::clone);
let page_index_policy = options.map(|o| o.column_index_policy());
DFParquetMetadata::new(&self.store, &object_meta)
.with_decryption_properties(file_decryption_properties)
...
```
Comet compiles that path in — `native/core/Cargo.toml` has `datafusion = { workspace = true, features = ["parquet_encryption", "sql"] }` — and `init_datasource_exec` sets `with_encryption_factory(...)` a dozen lines above this factory's wiring. So an encrypted-Parquet scan should now fail its metadata fetch rather than degrade gracefully.
Suggest keeping the forwarding and overriding only the page-index policy, so that override stays the single intentional deviation from the upstream factory:
```rust
fn get_metadata<'a>(
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {
let object_meta = self.partitioned_file.object_meta.clone();
let metadata_cache = Arc::clone(&self.metadata_cache);
let store = Arc::clone(&self.store);
let metadata_size_hint = self.metadata_size_hint;
let file_decryption_properties = options
.and_then(|o| o.file_decryption_properties())
.map(Arc::clone);
async move {
DFParquetMetadata::new(store.as_ref(), &object_meta)
.with_decryption_properties(file_decryption_properties)
.with_file_metadata_cache(Some(metadata_cache))
.with_metadata_size_hint(metadata_size_hint)
// Deliberately ignore the caller-requested policy; see module docs.
.with_page_index_policy(Some(PageIndexPolicy::Optional))
.fetch_metadata()
.await
.map_err(|e| { /* unchanged */ })
}
.boxed()
}
```
(DataFusion gates its block on the `parquet_encryption` feature of its own crate; `datafusion-comet-core` has no equivalent crate feature, so this can be called unconditionally here.)
Also worth confirming whether any existing encrypted-Parquet test actually exercises `init_datasource_exec` — the updated `caches_full_metadata_with_page_index` test won't catch this one.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: work around DataFusion 54.1.0 Parquet page-index regression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5132:
URL: https://github.com/apache/datafusion-comet/pull/5132
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: work around DataFusion 54.1.0 Parquet page-index regression [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5132:
URL: https://github.com/apache/datafusion-comet/pull/5132#issuecomment-5122656727
Benchmarked `main` against this PR branch with TPC-DS SF 1000:
<img width="3500" height="600" alt="tpcds_queries_compare" src="https://github.com/user-attachments/assets/9d2f1e62-602d-4411-9601-739a948af0c7" />
<img width="1000" height="600" alt="tpcds_allqueries" src="https://github.com/user-attachments/assets/6be6d4e2-ec58-44a2-b4e9-f9963374f681" />
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: work around DataFusion 54.1.0 Parquet page-index regression [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5132:
URL: https://github.com/apache/datafusion-comet/pull/5132#issuecomment-5122755486
Q88 on main:
<img width="760" height="621" alt="Screenshot 2026-07-29 at 3 44 29 PM" src="https://github.com/user-attachments/assets/68119ec9-6830-47cf-bfca-335aa7b5fb7f" />
this PR:
<img width="755" height="607" alt="Screenshot 2026-07-29 at 3 44 35 PM" src="https://github.com/user-attachments/assets/f5da3059-7283-42e5-8b10-7f1bdc4677c5" />
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org