zhuqi-lucas commented on code in PR #11168:
URL: https://github.com/apache/arrow-rs/pull/11168#discussion_r4078712240
##########
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:
Good catch, this was real. Fixed in 593fc9e13.
The skip path now asks the reader first:
`PageReader::supports_deferred_dictionary` defaults to `false`, and a reader
that does not opt in keeps the eager `read_dictionary_page` it had before.
Skipping past a dictionary a reader cannot hand back loses it, so the default
had to be the conservative one rather than a no-op `take_deferred_dictionary`.
New test `a_reader_that_cannot_defer_still_gets_its_dictionary` drives a
`PageReader` that forwards only the pre-existing methods and takes both new
defaults. Removing the gate makes it panic at `decoder.rs:213`, which is
exactly where you said it would.
##########
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:
Same root cause as the page.rs comment, fixed the same way in 593fc9e13:
this branch now reads the dictionary eagerly unless
`self.page_reader.supports_deferred_dictionary()` says the reader can return it
later.
`take_deferred_dictionary` keeping its `None => Ok(())` arm is deliberate —
on the read path it is reached whenever a dictionary-encoded data page appears,
including when the dictionary was already installed eagerly, so `None` there
means "nothing was deferred", not "something was lost". With the gate in place
that distinction is sound.
--
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]
