Copilot commented on code in PR #11168:
URL: https://github.com/apache/arrow-rs/pull/11168#discussion_r4078642576
##########
parquet/src/column/page.rs:
##########
@@ -404,6 +404,22 @@ pub trait PageReader: Iterator<Item = Result<Page>> + Send
{
/// column index information
fn skip_next_page(&mut self) -> Result<()>;
+ /// Decodes and returns a dictionary page this reader has previously
+ /// skipped past, if any.
+ ///
+ /// [`Self::skip_next_page`] may skip a dictionary page without decoding
+ /// it, since skipping rows never needs dictionary contents. If decoding
+ /// later reaches a dictionary-encoded data page, the reader is asked for
+ /// the dictionary through this method, which pays the deferred
+ /// decompression exactly once. A chunk skipped end to end never pays it.
+ ///
+ /// The default implementation returns `Ok(None)`, meaning the reader
+ /// never defers: every dictionary page it consumes is returned through
+ /// [`Self::get_next_page`] as before.
+ fn take_deferred_dictionary(&mut self) -> Result<Option<Page>> {
+ Ok(None)
Review Comment:
This default is not behaviorally compatible with existing `PageReader`
implementations. `skip_records` now consumes a dictionary via `skip_next_page`,
but readers such as `InMemoryPageReader` only advance past it and this method
returns `None`; the following dictionary-encoded data page then reaches
`set_data` without `set_dict` and panics at `Decoder for dict should have been
set`. Please either preserve/restore the skipped dictionary for non-deferred
readers or provide an eager compatibility path; a no-op default cannot make
existing custom implementations correct.
##########
parquet/src/column/reader.rs:
##########
@@ -317,9 +317,15 @@ where
return Ok(num_records - remaining_records);
};
- // If dictionary, we must read it
+ // If dictionary, skip it without decoding: skipping rows
+ // never needs dictionary contents (value skips only advance
+ // the index cursor, whole-page skips touch nothing). The page
+ // reader keeps the page's location, so a later data page that
+ // does need the dictionary still gets it, paying the deferred
+ // decompression exactly once. A chunk skipped end to end
+ // never pays it. See `PageReader::take_deferred_dictionary`.
if metadata.is_dict {
- self.read_dictionary_page()?;
+ self.page_reader.skip_next_page()?;
Review Comment:
This now consumes a dictionary page for every `PageReader`, but the new
`take_deferred_dictionary` default returns `None`. For example,
`InMemoryPageReader::skip_next_page` drops the `Page::DictionaryPage`; a
subsequent dictionary-encoded data page then reaches
`ColumnValueDecoderImpl::set_data` without an installed decoder and panics at
`decoder.rs:210-213`. Either make every `PageReader` retain skipped
dictionaries (including existing/custom implementations) or gate this path on
readers that support deferral.
##########
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:
The offset-index state still has no way to represent an offset-less
dictionary page. `dictionary_page` is only synthesized when the first
`PageLocation` differs from `ColumnChunkMetaData::byte_range().0`; when
`dictionary_page_offset` is absent, `byte_range()` starts at
`data_page_offset`, which is the first data-page location, so this branch
skips/reads data pages without ever retaining the leading dictionary. Such a
file will fail once a dictionary-encoded data page is decoded, despite the
stated support for dictionaries whose metadata offset is missing. The
page-index path needs an actual leading-header check (or must fall back to the
values state) before applying this deferral.
--
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]
