etseidl commented on code in PR #11168:
URL: https://github.com/apache/arrow-rs/pull/11168#discussion_r4123600555
##########
parquet/src/column/page.rs:
##########
@@ -404,6 +404,39 @@ 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.
+ ///
+ /// Returns `true` if [`Self::skip_next_page`] retains a dictionary page it
+ /// skips past, so that [`Self::take_deferred_dictionary`] can recover it.
+ ///
+ /// Readers that return `false` — the default, and therefore every existing
+ /// implementation — keep the behaviour they had before deferral existed:
+ /// the column reader reads and installs the dictionary eagerly rather than
+ /// skipping past it. This matters because a skipped dictionary that cannot
+ /// be recovered is simply lost, and the first dictionary-encoded data page
+ /// decoded afterwards would have no dictionary to decode against.
+ ///
+ /// An implementation that returns `true` must ensure every dictionary page
+ /// passed to [`Self::skip_next_page`] is recoverable, for as long as the
+ /// column chunk is being read.
Review Comment:
```suggestion
/// Returns whether skipped dictionary pages can later be recovered with
/// [`Self::take_deferred_dictionary`].
///
/// Implementations returning `true` must retain dictionary pages consumed by
/// [`Self::skip_next_page`]. The default preserves compatibility with
existing
/// readers by requiring dictionaries to be decoded eagerly.
```
##########
parquet/src/column/page.rs:
##########
@@ -404,6 +404,39 @@ 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.
+ ///
+ /// Returns `true` if [`Self::skip_next_page`] retains a dictionary page it
+ /// skips past, so that [`Self::take_deferred_dictionary`] can recover it.
+ ///
+ /// Readers that return `false` — the default, and therefore every existing
+ /// implementation — keep the behaviour they had before deferral existed:
+ /// the column reader reads and installs the dictionary eagerly rather than
+ /// skipping past it. This matters because a skipped dictionary that cannot
+ /// be recovered is simply lost, and the first dictionary-encoded data page
+ /// decoded afterwards would have no dictionary to decode against.
+ ///
+ /// An implementation that returns `true` must ensure every dictionary page
+ /// passed to [`Self::skip_next_page`] is recoverable, for as long as the
+ /// column chunk is being read.
+ fn supports_deferred_dictionary(&self) -> bool {
+ false
+ }
+
+ /// 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.
Review Comment:
```suggestion
/// Returns and removes a previously skipped dictionary page, if any.
///
/// The default implementation returns `Ok(None)`, meaning the reader
/// never defers.
```
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -531,6 +531,41 @@ pub(crate) fn decode_page(
Ok(result)
}
+/// A dictionary page this reader skipped past without decoding, kept as a
+/// location so it can still be decoded if a later data page turns out to
+/// need it.
Review Comment:
```suggestion
/// Location of a dictionary page skipped without decoding.
```
The diagram and description could probably be replaced with a link to the
PR, but I'm ok with leaving it in.
##########
parquet/src/column/reader.rs:
##########
@@ -421,6 +440,28 @@ where
}
}
+ /// Installs a dictionary page the page reader skipped past, if this
+ /// column turns out to need one after all. A no-op when nothing was
+ /// deferred: the eager path (a dictionary page arriving through
+ /// `get_next_page`) and the deferred path are mutually exclusive, since
+ /// a skipped page is never returned by `get_next_page` and vice versa.
Review Comment:
```suggestion
/// Installs a dictionary previously deferred by the page reader.
/// A no-op when nothing was deferred.
```
##########
parquet/src/column/reader.rs:
##########
@@ -317,9 +317,24 @@ where
return Ok(num_records - remaining_records);
};
- // If dictionary, we must read it
+ // If dictionary, skip it without decoding *when the page
+ // reader can give it back later*: skipping rows never needs
+ // dictionary contents (value skips only advance the index
+ // cursor, whole-page skips touch nothing), so a chunk skipped
+ // end to end never pays the decompression, and a later data
+ // page that does need the dictionary pays it exactly once.
+ //
+ // A reader that does not retain skipped dictionaries must
+ // still be given the eager path: skipping past a dictionary it
+ // cannot return would drop it, and the next dictionary-encoded
+ // data page would have nothing to decode against. See
+ // `PageReader::supports_deferred_dictionary`.
Review Comment:
```suggestion
// Defer dictionary decoding when the page reader can
recover it later.
// Otherwise, preserve the eager behavior required by
existing readers.
```
The comment obscures the code, which is the whole point of the PR. I missed
this on my first pass.
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1151,20 +1211,77 @@ 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)
} else {
// If no dictionary page exists, simply pop the data page
from page_locations
if page_locations.pop_front().is_some() {
*page_index += 1;
}
+ None
}
-
- Ok(())
}
+ };
+ if deferred.is_some() {
+ self.deferred_dictionary = deferred;
+ }
+ Ok(())
+ }
+
+ fn supports_deferred_dictionary(&self) -> bool {
+ match &self.state {
+ // Page headers are read as the chunk is walked, so a dictionary is
+ // recognised by its header type whether or not the column metadata
+ // recorded a `dictionary_page_offset` for it.
Review Comment:
```suggestion
// Walking headers identifies dictionaries independently of
metadata.
```
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1102,23 +1142,36 @@ impl<R: ChunkReader> PageReader for
SerializedPageReader<R> {
}
fn skip_next_page(&mut self) -> Result<()> {
- match &mut self.state {
+ // Skipping a dictionary page records where it was instead of dropping
+ // it: only value decoding ever needs dictionary contents, so the page
+ // body stays untouched until a data page turns out to need it. See
+ // [`DeferredDictionaryPage`] for the state diagram.
Review Comment:
```suggestion
// Retain skipped dictionary pages so they can be decoded on demand.
// See [`DeferredDictionaryPage`] for the state diagram.
```
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1151,20 +1211,77 @@ 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)
} else {
// If no dictionary page exists, simply pop the data page
from page_locations
if page_locations.pop_front().is_some() {
*page_index += 1;
}
+ None
}
-
- Ok(())
}
+ };
+ if deferred.is_some() {
+ self.deferred_dictionary = deferred;
+ }
+ Ok(())
+ }
+
+ fn supports_deferred_dictionary(&self) -> bool {
+ match &self.state {
+ // Page headers are read as the chunk is walked, so a dictionary is
+ // recognised by its header type whether or not the column metadata
+ // recorded a `dictionary_page_offset` for it.
+ SerializedPageReaderState::Values { .. } => true,
+ // The offset-index state can only represent a dictionary the
+ // metadata gave an offset for: `dictionary_page` is synthesised
+ // from the gap between `byte_range().0` and the first page
+ // location, and when `dictionary_page_offset` is absent those are
+ // the same address. A dictionary inlined ahead of the first data
+ // page is then indistinguishable from a data page location, so
+ // skipping it would drop it. Decline to defer in that case and let
+ // the column reader install it eagerly instead.
Review Comment:
```suggestion
// With an offset index, a dictionary is recoverable only when
its
// location can be inferred before the first data-page location,
in
// which case `dictionary_page` will be `Some`.
```
--
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]
