etseidl commented on code in PR #11157:
URL: https://github.com/apache/arrow-rs/pull/11157#discussion_r4146134082
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -774,6 +776,47 @@ impl ArrowReaderOptions {
self
}
+ /// Sets the same [`ColumnChunkMask`] for both page-index structures.
+ pub fn with_page_index_mask(self, mask: ColumnChunkMask) -> Self {
+ self.with_column_index_mask(mask.clone())
+ .with_offset_index_mask(mask)
+ }
+
+ /// Sets the [`ColumnChunkMask`] for the Parquet [ColumnIndex] structure.
+ ///
+ /// The column index can be costly to decode and store, especially when it
is needed
+ /// only for a subset of row groups or columns (such as when filtering by
a predicate
+ /// on a single column). Providing a [`ColumnChunkMask`] can greatly
decrease
+ /// the time needed to decode this metadata.
+ ///
+ /// The mask applies only if the column-index policy is not
[`PageIndexPolicy::Skip`]
+ /// (the default), or an underlying reader is configured to preload the
index. It is
+ /// honored by loading APIs such as [`ArrowReaderMetadata::load`];
+ /// [`ArrowReaderMetadata::try_new`] does not load or filter page indexes.
+ ///
+ /// [ColumnIndex]:
https://github.com/apache/parquet-format/blob/master/PageIndex.md
Review Comment:
My original design was something similar, but instead added three new
variants to the policy which allowed for column only selection, row only
selection, or both for a grid. The `ColumnChunkMask` grew out of that as a way
to get the behavior without a breaking API change. _I_ didn't want to code that
up, but LLMs make the plumbing changes too easy 😅.
Anyway, I agree that's a cleaner solution that requires less added baggage.
I guess the question is how soon do we want this? Given the work @mkleen is
doing with direct parsing of the column index in #11285, maybe there's less
urgency here and we can wait for the breaking window to open for 61.0.0.
I'm not in love with option 2.
Tangent: since the change to optional cells, I've wondered if we still need
the `Required` vs `Optional` distinction any longer. The policy enum was
introduced when offset indexes were all or nothing, and we needed to decide
what to do if a file lacked some, but not all, cells since there was no way to
have a placeholder for a missing cell. Now that each cell is optional, the
offset index behavior can mirror the column index. If we're waiting for a
breaking change, perhaps we can deprecate `Required` and only support
`Optional` 🤷
--
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]
