Skip to content

fix: [branch-1.0] work around DataFusion 54.1.0 Parquet page-index regression (#5132) - #5142

Merged
andygrove merged 1 commit into
apache:branch-1.0from
andygrove:backport-5132-branch-1.0
Jul 30, 2026
Merged

fix: [branch-1.0] work around DataFusion 54.1.0 Parquet page-index regression (#5132)#5142
andygrove merged 1 commit into
apache:branch-1.0from
andygrove:backport-5132-branch-1.0

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 perf: cache full Parquet metadata (incl. page index) via DataFusion's CachedParquetFileReaderFactory #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.

@andygrove andygrove changed the title fix: work around DataFusion 54.1.0 Parquet page-index regression (#5132) fix: [branch-1.0] work around DataFusion 54.1.0 Parquet page-index regression (#5132) Jul 29, 2026

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport, @andygrove!

@andygrove
andygrove merged commit 60becf7 into apache:branch-1.0 Jul 30, 2026
137 of 138 checks passed
@andygrove
andygrove deleted the backport-5132-branch-1.0 branch July 30, 2026 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants