zhuqi-lucas commented on code in PR #11168:
URL: https://github.com/apache/arrow-rs/pull/11168#discussion_r4078714531
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1151,18 +1207,55 @@ impl<R: ChunkReader> PageReader for
SerializedPageReader<R> {
..
} => {
if dictionary_page.is_some() {
- // If a dictionary page exists, consume it by taking it
(sets to None)
- dictionary_page.take();
+ // Consume the dictionary page, keeping its location.
+
dictionary_page.take().map(DeferredDictionaryPage::Location)
Review Comment:
Right about the state, and the same gate covers it.
`SerializedPageReader::supports_deferred_dictionary` now answers per state and
returns `false` for `Pages` when `dictionary_page` is `None`, so that case
falls back to the eager read instead of skipping a location that may physically
be the dictionary.
Worth noting the synthesis itself is unchanged from `main` —
`dictionary_page` is derived from the gap between `byte_range().0` and the
first page location, and those coincide when `dictionary_page_offset` is
absent. Before this PR that was harmless because `get_next_page` returned the
leading page and `read_dictionary_page` installed it regardless of how it was
labelled. Deferring is what would have made it fail, so declining to defer
restores the previous behaviour rather than papering over it. Teaching the
offset-index state to recognise a leading dictionary by header is a real
improvement but is orthogonal to this PR, and I would rather not widen it here.
Covered by
`offset_index_state_without_a_dictionary_offset_declines_deferral`, alongside
the two states where deferral is safe.
--
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]
