You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Defer dictionary page decode on the skip path - #11168
GenericColumnReader::skip_records decompresses a column chunk's dictionary page as soon as it reaches it, even when every remaining page is then skipped and no value from the chunk is ever decoded. Skipping values needs only the RLE index cursor and skipping whole pages needs nothing, so dictionary contents are only ever needed by value decoding. On zstd-compressed data with large dictionaries this is a large, avoidable cost, and it is most visible with pushdown_filters/RowSelection and with predicate caching, where output columns are consumed largely by skipping.
A local timing test on a zstd column whose dictionary holds 40k distinct strings (14,800 compressed bytes):
whole-chunk skip, dictionary never decoded
84 µs
partial skip that pays the deferred dictionary + first page
4.76 ms
No regression on the plain read path. The read path gains one matches! on the page encoding and one always-Nonetake() per data page; arrow_reader's dictionary-encoded read benchmarks are all within the noise threshold against main:
dictionary-encoded read
change vs main
Int32, mandatory
+0.43%
Int32, optional
+0.58%
Int32, optional, half NULLs
+0.72%
UInt8, mandatory
+0.28%
UInt8, optional
+0.35%
(Run locally with --features arrow,test_common,experimental; CodSpeed does not run on fork PRs.)
What changes are included in this PR?
The deferral is created on the skip path and consumed on the read path; that
crossing is the whole change:
flowchart TD
A["skip_records reaches a dictionary page"] --> G{"supports_deferred_dictionary()?"}
G -->|"false (trait default)"| H["read_dictionary_page()<br/>eager, unchanged from main"]
G -->|"true"| B["record data_start + header<br/>no decompress"]
B -.->|"deferred, may never fire"| C["read_records: a data page needs values"]
C --> D["take_deferred_dictionary()"]
D --> E["get_bytes(data_start)<br/>decode_page, set_dict"]
B ==>|"chunk ends first"| F["never decompressed"]
Loading
The skip path records the dictionary page's location (DeferredDictionaryPage) instead of decoding it. A data page that turns out to need the dictionary installs it then, paying the decompression exactly once; a chunk skipped end to end never pays it.
PageReader::supports_deferred_dictionary, a new trait method defaulting to false, decides whether the skip path may skip past a dictionary at all. A reader that does not opt in keeps the eager path exactly as before — skipping past a dictionary it cannot hand back would simply lose it. SerializedPageReader answers per state: it can always defer while walking page headers, and declines in the offset-index state when the metadata recorded no dictionary_page_offset, because there a dictionary inlined ahead of the first data page is indistinguishable from a data page location.
PageReader::take_deferred_dictionary, a new trait method defaulting to Ok(None), returns a dictionary the reader skipped past.
Whether a page is a dictionary is judged by its actual header type, not the dictionary_page_offset metadata flag, so files that inline the dictionary without recording an offset (older parquet-mr writers, e.g. the one behind test_read_nested_lists) are handled correctly.
On the re-read. The deferred install is a ChunkReader::get_read at the
recorded offset, which is already part of the trait. It never adds I/O relative
to main: these are the same bytes the eager path would have read, either read
later or, when the chunk is skipped end to end, not at all. On the async path it
is not a fetch at all — the column chunk is already buffered, so the read is an
in-memory slice.
Are these changes tested?
Yes. The deferral itself:
whole-chunk skip never decodes the dictionary (corrupting the dictionary body leaves the skip green);
a partial skip pays it (the same corruption then fails);
skip-then-read returns correct values;
alternating skip and read — the shape a RowSelection produces — triggers the deferral more than once per chunk;
the plain read path is unchanged;
a timing test prints the avoided cost.
Page readers that cannot defer:
a reader forwarding only the pre-existing trait methods still gets its dictionary, through both a partial and a whole-chunk skip. Removing the supports_deferred_dictionary gate makes it panic in set_data;
a reader forwarding take_deferred_dictionary but not supports_deferred_dictionary — the easy mistake when adding both to an existing wrapper — stays correct by falling back to the eager path.
The probe's answer in all three SerializedPageReader states, including the offset-index state without a dictionary_page_offset, where it declines. The existing test_read_nested_lists (nested column + RowSelection) caught the offset-less-dictionary case during development and still covers it.
Are there any user-facing changes?
No format change, and no behavior change for existing PageReader implementations: deferral is opt-in through supports_deferred_dictionary, which defaults to false, so a reader that does not implement it keeps the eager dictionary read it had before. PageReader gains two defaulted trait methods; an implementation that wraps another PageReader should forward both to benefit, and stays correct if it forwards neither. Forwarding only take_deferred_dictionary without supports_deferred_dictionary is also safe — the skip path simply stays eager.
One observable difference for a reader that does opt in: a corrupt dictionary page is no longer guaranteed to raise an error. Today the page is decompressed the moment the skip path reaches it, so a corrupt one fails even for a column chunk that goes on to decode nothing. With the deferral, a chunk skipped end to end never touches those bytes and the error never surfaces. Any chunk that decodes a value still hits it, just later. This is not a correctness change — no wrong data is returned either way — but it does mean corruption in a never-read dictionary now goes unreported. skipping_a_whole_chunk_never_decodes_the_dictionary asserts exactly this, using a deliberately corrupted dictionary body as the probe for "was it decompressed".
`GenericColumnReader::skip_records` decompressed a column chunk's
dictionary page the moment it reached it, even when every remaining page
was then skipped and no value was ever decoded. On zstd-compressed data
with large dictionaries this is a large, avoidable cost: skipping values
needs only the RLE index cursor, and skipping whole pages needs nothing,
so dictionary contents are only ever needed by value decoding.
The skip path now records the dictionary page's location instead of
decoding it (`DeferredDictionaryPage`), and a data page that turns out to
need the dictionary installs it then, paying the decompression exactly
once. A chunk skipped end to end never pays it. The re-read is a random
`ChunkReader::get_read` at the recorded offset, which the trait already
guarantees and which is an in-memory slice on the async path.
`PageReader::take_deferred_dictionary` is a new trait method defaulting
to `Ok(None)`, so a reader that never defers is unchanged, and the eager
path (a dictionary page arriving through `get_next_page`) is untouched.
Whether a page is a dictionary is judged by its actual header type, not
the `dictionary_page_offset` metadata flag, so files that inline the
dictionary without recording an offset (older parquet-mr writers, as in
`test_read_nested_lists`) are handled correctly.
Tests: whole-chunk skip never decodes the dictionary (corrupting the
dictionary body leaves the skip green), a partial skip pays it (the same
corruption fails), skip-then-read returns correct values, and the plain
read path is unchanged. A timing test prints the avoided cost.
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench
Results will be posted here when complete
The three select-only cases skip the leading data page, which now defers
the dictionary page rather than decoding it up front, so the dictionary
is read when the following data page is first decoded rather than before
it. Same requests and same bytes, order swapped. Snapshots updated and
annotated; caught by `cargo test -p parquet` (the workspace test target)
where the earlier `--lib` run did not reach these integration tests.
skip_records skipped past a dictionary page for every PageReader, but only
SerializedPageReader retains one. Any other implementation lost it, and the
defaulted take_deferred_dictionary returned Ok(None), so the next
dictionary-encoded data page reached set_data with no decoder and panicked.
Add PageReader::supports_deferred_dictionary, defaulting to false, and keep
the eager read_dictionary_page path for readers that do not opt in.
SerializedPageReader answers per state. The offset-index state can only
represent a dictionary whose offset the metadata recorded: dictionary_page is
synthesised from the gap between byte_range().0 and the first page location,
and those coincide when dictionary_page_offset is absent, so a dictionary
inlined ahead of the first data page is indistinguishable from a data page
location. It declines deferral there.
Tests cover a non-deferring reader through both a partial and a whole-chunk
skip, and the probe's answer in all three states.
Alternating skip and read is the shape a RowSelection produces and exercises
the deferral more than once per chunk; nothing covered it.
A wrapper that forwards take_deferred_dictionary but not
supports_deferred_dictionary is the easy mistake when adding both methods to
an existing implementation. It stays correct — the probe reports false, the
skip path reads eagerly, and the forwarded method never has anything to
return — and that is now asserted rather than only claimed in the docs.
Also notes on the offset-index probe test that the case it guards is unlikely
to be reachable: a writer old enough to omit dictionary_page_offset predates
the offset index, so it would not emit page locations either.
Validate compressed page size after accounting for header length
parquet/src/file/serialized_reader.rs:1184
On this branch, remaining_bytes still includes header_len, but compressed_page_size is validated against the unadjusted total before subtracting header_len + data_page_size. A malformed header with compressed_page_size <= remaining_bytes but header_len + compressed_page_size > remaining_bytes can therefore underflow the u64 (or panic in debug) instead of returning the invalid-page error that get_next_page returns. Validate against *remaining_bytes - header_len as u64 after verify_page_header_len.
The diagram on DeferredDictionaryPage had NotSeen -> Deferred happening
unconditionally on a skip, which stopped being true once the transition was
gated on supports_deferred_dictionary. A reader that cannot defer now takes a
third path straight to Decoded, and that path is the one keeping existing
PageReader implementations correct, so it belongs in the picture.
… merge
Patches every arrow-rs 60.0.0 crate to zhuqi-lucas/arrow-rs@f18aaab so the
DataFusion benchmarks can measure deferring dictionary page decode on the
Parquet skip path end to end, through pushdown_filters and the predicate
cache, before the upstream PR is marked ready for review.
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench
Results will be posted here when complete
cc @etseidl@alamb@adriangb this new design for skip page logic ready for a look. It defers dictionary page decode on the skip path so a column chunk that is skipped end to end never decompresses its dictionary; a chunk that is read after a partial skip pays it on the first data page instead.
And some benchmark is pretty good, and also clickbench here show good result:
read_page_header_len_from_bytes moved out of the SerializedPageReader impl
and became a free function, but only one of its two call sites was updated;
the other still went through Self:: and stopped compiling.
Thanks @etseidl — applied all of them, the docs read much better trimmed. One heads-up: applying the suggestions moved read_page_header_len_from_bytes out of the SerializedPageReader impl and updated only one of its two call sites, so the branch went red for a moment. Fixed in d2b1294.
You are right about the corrupt dictionary, and it is sharper than "may not trigger an error" — it definitely will not, for any chunk that decodes nothing. Today the page is decompressed the moment the skip path reaches it, so corruption fails even when the chunk goes on to decode zero values; with the deferral those bytes are never touched. A chunk that does decode something still hits the error, just later. No wrong data is returned either way, but corruption in a never-read dictionary now goes unreported.
It is also the mechanism behind two of the tests: corrupt_dictionary_body zero-fills the dictionary page body, and the same corrupted file makes skipping_a_whole_chunk_never_decodes_the_dictionary pass while a_partial_skip_pays_the_deferred_dictionary fails — "does this error" is the probe for "was it decompressed". I have written the behaviour into the user-facing changes section.
Copilot caught that `supports_deferred_dictionary` opened by describing a
method that decodes and returns a page, while it only returns a bool. The
paragraph belongs to `take_deferred_dictionary`, which previously had two
lines and no explanation of why deferring is safe.
The reason will be displayed to describe this comment to others. Learn more.
I think the comment here that explains the changes in this PR will be confusing in the future
Maybe a comment saying that "DictionaryPages are deferred until first read, see #11154" would help, but the fact that in the past the behavior was different is likely not relevant in the future
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alamb — agreed, the before/after framing stops meaning anything once this merges. Reworded all three to state what the reader does, and to explain the ordering the snapshot actually shows: the dictionary trails the data page wherever a skip precedes it, and leads it where none does. Issue link kept.
The notes described the before/after of this PR, which stops meaning
anything once it is merged. Describe what the reader does instead, and
explain the ordering the snapshot actually shows: a dictionary trails the
data page wherever a skip precedes it, and leads it where none does.
Thanks @alamb — yes, ready from my side. Since @etseidl's approval the only change is the comment reword you asked for above.
One behaviour change worth naming before merge: a corrupt dictionary in a chunk that is never read no longer errors, since those bytes are never touched. No wrong data is returned — anything that decodes a value still decodes the dictionary and still errors. Covered by skipping_a_whole_chunk_never_decodes_the_dictionary and a_partial_skip_pays_the_deferred_dictionary.
- Move `lazy_dictionary_timings` (an ignored test that only printed
timings) to a criterion benchmark, `benches/dictionary_skip.rs`.
- Drop the doc sentence about the pre-change default.
- Say that `take_deferred_dictionary` returns the decoded dictionary `Page`.
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
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.
alamb
left a comment
Which issue does this PR close?
skip_recordseagerly decompresses dictionary pages for column chunks it never decodes #11154.Rationale for this change
GenericColumnReader::skip_recordsdecompresses a column chunk's dictionary page as soon as it reaches it, even when every remaining page is then skipped and no value from the chunk is ever decoded. Skipping values needs only the RLE index cursor and skipping whole pages needs nothing, so dictionary contents are only ever needed by value decoding. On zstd-compressed data with large dictionaries this is a large, avoidable cost, and it is most visible withpushdown_filters/RowSelectionand with predicate caching, where output columns are consumed largely by skipping.A local timing test on a zstd column whose dictionary holds 40k distinct strings (14,800 compressed bytes):
No regression on the plain read path. The read path gains one
matches!on the page encoding and one always-Nonetake()per data page;arrow_reader's dictionary-encoded read benchmarks are all within the noise threshold againstmain:(Run locally with
--features arrow,test_common,experimental; CodSpeed does not run on fork PRs.)What changes are included in this PR?
The deferral is created on the skip path and consumed on the read path; that
crossing is the whole change:
flowchart TD A["skip_records reaches a dictionary page"] --> G{"supports_deferred_dictionary()?"} G -->|"false (trait default)"| H["read_dictionary_page()<br/>eager, unchanged from main"] G -->|"true"| B["record data_start + header<br/>no decompress"] B -.->|"deferred, may never fire"| C["read_records: a data page needs values"] C --> D["take_deferred_dictionary()"] D --> E["get_bytes(data_start)<br/>decode_page, set_dict"] B ==>|"chunk ends first"| F["never decompressed"]DeferredDictionaryPage) instead of decoding it. A data page that turns out to need the dictionary installs it then, paying the decompression exactly once; a chunk skipped end to end never pays it.PageReader::supports_deferred_dictionary, a new trait method defaulting tofalse, decides whether the skip path may skip past a dictionary at all. A reader that does not opt in keeps the eager path exactly as before — skipping past a dictionary it cannot hand back would simply lose it.SerializedPageReaderanswers per state: it can always defer while walking page headers, and declines in the offset-index state when the metadata recorded nodictionary_page_offset, because there a dictionary inlined ahead of the first data page is indistinguishable from a data page location.PageReader::take_deferred_dictionary, a new trait method defaulting toOk(None), returns a dictionary the reader skipped past.dictionary_page_offsetmetadata flag, so files that inline the dictionary without recording an offset (older parquet-mr writers, e.g. the one behindtest_read_nested_lists) are handled correctly.On the re-read. The deferred install is a
ChunkReader::get_readat therecorded offset, which is already part of the trait. It never adds I/O relative
to
main: these are the same bytes the eager path would have read, either readlater or, when the chunk is skipped end to end, not at all. On the async path it
is not a fetch at all — the column chunk is already buffered, so the read is an
in-memory slice.
Are these changes tested?
Yes. The deferral itself:
RowSelectionproduces — triggers the deferral more than once per chunk;Page readers that cannot defer:
supports_deferred_dictionarygate makes it panic inset_data;take_deferred_dictionarybut notsupports_deferred_dictionary— the easy mistake when adding both to an existing wrapper — stays correct by falling back to the eager path.The probe's answer in all three
SerializedPageReaderstates, including the offset-index state without adictionary_page_offset, where it declines. The existingtest_read_nested_lists(nested column +RowSelection) caught the offset-less-dictionary case during development and still covers it.Are there any user-facing changes?
No format change, and no behavior change for existing
PageReaderimplementations: deferral is opt-in throughsupports_deferred_dictionary, which defaults tofalse, so a reader that does not implement it keeps the eager dictionary read it had before.PageReadergains two defaulted trait methods; an implementation that wraps anotherPageReadershould forward both to benefit, and stays correct if it forwards neither. Forwarding onlytake_deferred_dictionarywithoutsupports_deferred_dictionaryis also safe — the skip path simply stays eager.One observable difference for a reader that does opt in: a corrupt dictionary page is no longer guaranteed to raise an error. Today the page is decompressed the moment the skip path reaches it, so a corrupt one fails even for a column chunk that goes on to decode nothing. With the deferral, a chunk skipped end to end never touches those bytes and the error never surfaces. Any chunk that decodes a value still hits it, just later. This is not a correctness change — no wrong data is returned either way — but it does mean corruption in a never-read dictionary now goes unreported.
skipping_a_whole_chunk_never_decodes_the_dictionaryasserts exactly this, using a deliberately corrupted dictionary body as the probe for "was it decompressed".