fix(reader): treat the parquet page index as optional - #10
Merged
Conversation
The plan-time opener loaded footers with PageIndexPolicy::Required, so a scan failed outright with "missing offset index" on any file that advertises a page index while one of its column chunks carries no offset index. Valid parquet is free to have that shape, and readers ingesting files written elsewhere cannot assume otherwise. The index is an optimization, not a correctness requirement. It feeds page-level pruning, which already no-ops when either index is absent, and row selection without an offset index simply fetches whole column chunks. Optional keeps both behaviours when the index is present and degrades instead of failing when it is not — matching upstream DataFusion's own parquet source, and this crate's metadata cache, which already used Optional.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Change is correct and minimal: nothing downstream depends on the offset index — prune_plan_with_page_index no-ops when either index is absent, and LiquidStreamBuilder applies the RowSelection after decode rather than via page locations. It also brings the opener in line with the Optional policy already used by the metadata cache in plantime/source.rs:153.
Test fixture is well-constructed: rewriting only the footer over a byte-identical prefix keeps the surviving column-index pointers valid, and fixture_is_rejected_by_required_policy pins the exact rejection reason so the scan test can't pass vacuously.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The plan-time opener loads parquet footers with
PageIndexPolicy::Required. That fails the entire scan withParquet error: missing offset indexon a file that advertises a page index while one of its column chunks carries no offset index.That shape is valid parquet. Any reader that ingests files written by other tools has to tolerate it — the page index is an optional footer structure, and nothing guarantees uniform coverage across column chunks.
Note that a file with no page index at all is unaffected: when no column chunk declares an index range,
range_for_page_indexreturnsNoneand page index parsing is skipped entirely, so evenRequiredaccepts it. The failure needs an index range to exist alongside a missing offset-index pointer.Why
Optionalis correct, not just lenientThe page index is an optimization:
page_filter.rschecksoffset_index().is_none() || column_index().is_none()and returns the access plan unchanged).Optional's degrade path is explicit: on this exact error the parser clears both indexes and returnsOk.So
Optionalkeeps every existing behaviour when the index is present, and gives up only page-level skipping when it is not — in place of failing the query.Two supporting points: upstream DataFusion's own parquet source uses
Optional, and this crate's metadata cache inplantime/source.rsalready did too, so the opener was inconsistent with both.Tests
src/datafusion-local/src/tests/page_index.rs.No writer exposes "column index but no offset index" as an option, so the fixture writes a fully indexed file, keeps its data pages and column-index blobs byte-for-byte, and re-encodes only the footer with each chunk's offset-index pointer cleared.
fixture_is_rejected_by_required_policyguards the premise — the fixture really is the shapeRequiredrejects, so the scan test cannot pass vacuously. This one runs anywhere.scans_file_without_offset_indexscans it through LiquidCache with a predicate (the thing that builds the page-pruning predicate), checks results on both the cold and warm read, and asserts the cache was actually populated so the test cannot silently stop covering the LiquidCache reader.Heads-up for review: the scan test needs
direct_io, like every other cache-backed test here, so it does not run on macOS. CI is the first place it executes.