A note on naming: this was originally PageIndexSelection, but it occurred to me that we might want to use this for the file metadata as well. I changed it to RowGroupColumnSelection, but that was a bit unwieldy. I asked Google for some naming suggestions, and this name made the most sense to me since the intersection of row group and column is a column chunk.

Drive by change. Accessors have been added for these, so they no longer need to be visible.

Docs / Bug (C3). These docs suggest a column index mask for the predicate columns and an offset index mask for the projected columns. With that setup, StatisticsConverter returns arrays of different lengths (see the review body):

CI columns([0]) (predicate), OI columns([1]) (projection): data_page_mins len 30, data_page_row_counts len 0

Page pruning needs the offset index of the predicate columns too. The suggestion also adds two other traps (C7): a mask without a policy does nothing, and ArrowReaderMetadata::try_new ignores the masks.

I think this one's getting a little pedantic 😅. I mean, I think users can figure out that a function that explicitly states that it won't load the page index won't be affected by options controlling the loading of the page index.

Fair point re noting that you'll want the offset index for the predicate columns as well as the projected columns.

API, one-way door (C4). An empty set selects everything. The common callers build the set from a list that can be empty:

columns([])                     -> loads 12 of 12 offset indexes   (SELECT count(*): no projected leaves)
row_groups(<no surviving rgs>)  -> loads 12 of 12                  (stats pruned every row group)
columns([-1])                   -> loads 12 of 12
columns([100])                  -> loads 0 of 12, page_index() = None
columns([-1, 100])              -> loads 0 of 12

The neighbour APIs do the opposite: ProjectionMask::leaves(schema, []) selects no leaves, with_row_groups(vec![]) reads nothing, and ParquetStatisticsPolicy::skip_except(&[]) is SkipAll. After a release, a change here silently changes what callers load. Suggestion: an empty set selects nothing, and all() stays the only way to select everything. test_chunk_mask (L1545-1549) and the constructor docs (L155-156, L166-167, L177-179) need the same change.

I'm fine with revisiting this. I think I was just trying to avoid users inadvertently specifying empty sets, but I agree that empty (as opposed to None) should mean just that.

API, one-way door (C5). The constructors take i32. The other APIs around this one use usize: includes_* here, ProjectionMask::leaves, with_row_groups, PageIndexProvider::column_index, ParquetStatisticsPolicy::skip_except. So every caller writes .map(|i| i as i32). That cast wraps silently, and a negative result then means "ignore" (and, with C4, "all"). A related detail: ColumnChunkMask::all().includes_column(u32::MAX as usize) is false.

Suggestion: take impl IntoIterator<Item = usize> in the public API. Keep i32/u32 storage internal if you want it. A change after release breaks every caller, so this is the time to decide.

Well, the issue is parquet doesn't allow arrays to be sized beyond i32::MAX. It was probably a mistake to use usize elsewhere, but I guess that horse is out of the barn. I'll rethink this.

Perf nit (C15). For each selected row group, this loop visits every column and does a BTreeSet lookup, also when the mask selects 1 of 10,000 columns. add_ranges in push_decoder.rs does the same, on every try_decode. A pub(crate) helper can iterate the selected set directly (compiled and tested on this head):

// reader.rs
impl ColumnChunkMask {
    /// Selected column indexes in `0..num_columns`, in order
    pub(crate) fn column_indices(
        &self,
        num_columns: usize,
    ) -> Box<dyn Iterator<Item = usize> + '_> {
        match &self.columns {
            None => Box::new(0..num_columns),
            Some(set) => {
                let end = i32::try_from(num_columns).unwrap_or(i32::MAX);
                Box::new(set.range(0..end).map(|&i| i as usize))
            }
        }
    }
}

// here, and in parse_offset_index / add_ranges
for col_idx in mask.column_indices(rg.num_columns()) {

Test gap (C14). No test sets a mask through ArrowReaderOptions or ParquetMetaDataReader::with_page_index_mask. Mutation testing (on the #11159 head, same code) shows it: ArrowReaderOptions::with_column_index_mask, with_offset_index_mask, both getters, and ParquetMetaDataReader::with_page_index_mask can each return Default::default(), and all tests still pass. If a refactor drops the mask plumbing, readers decode the full index again and no test fails. There, these two tests kill all 5 mutants. They pass on this head.

Also missing: a data read (push decoder or async) with a mask. That test would have found C2. Add it after the C2 fix.

Test gap (N1). test_chunk_mask is in mod async_tests, which has #[cfg(all(feature = "async", feature = "arrow", test))]. The default features of parquet do not include async, so cargo test -p parquet does not run it. The test uses no async code. Move it to mod tests (L977).

Nit (C16). Typo.

Nit (C16). start_offset is no longer a parameter.

Non-blocking simplification: now that the decoder again requires one buffer covering the entire selected index range, could this path resolve that buffer once and borrow subslices, as before? Each get_bytes here (and in parse_offset_index) scans PushBuffers and creates/drops a Bytes slice. Reusing the covering buffer would avoid repeated searches and reference-count operations while keeping the same I/O behavior. I have not measured a material regression, so this can be a follow-up.

Yeah, this can be reverted. I was hoping to at some point reintroduce more targeted ranges for the page index, but that may be over-engineering. I could see at least wanting one range for the column index and a second for the offset index if the column selection is small and the table very wide. Later work can handle that case.

done in 4823984

Optional performance follow-up: these boxed iterators add an allocation per selected row group when iterating columns, in both range discovery and parsing, plus dynamic dispatch during iteration. A small concrete iterator covering the all-indices and selected-indices cases could avoid that overhead. It would be useful to measure a many-row-group sparse-mask case before adding implementation complexity; this does not block approval.

I had Codex whip up some tests to show the difference. Added a concrete iterator in ad86518

this is a nice API 👍

nit is you can could reduce the indent with something like

        if policy == PageIndexPolicy::Skip {
          continue;
        }

tru

done in b9fd884

The interaction between PageIndexPolicy and the ColumnChunkMask is complicated, especially when there is an existing PageIndex in ParquetMetadata

One potential API we could consider is a new variant in PageIndexPolicy::Mask to avoid having to thread through another set of variables.

pub enum PageIndexPolicy {
    /// Do not read the page index.
    #[default]
    Skip,
...
    /// Ensure the specified indexes exist, and error if not
    Required(ColumnChunkMask),
    /// Load the specified indexes if they exist
    Optional(ColumnChunkMask),
}

However, that would be an API change as PageIndexPolicy isn't marked as non_exhastive so we would have to either

1. Wait for the next major release
2. Go with this API, and then unify PageIndexPolicy, and deprecate with_column_index_mask et al methods in next major version 🤔

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 🤷

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 🤷

I think at least at some point in the past the Required variant was to fail fast when a user didn't correctly configure / provide the PageIndex to the parquet reader and the caller wanted to ensure they were properly using the index. Maybe the APIs are sophisticated enough these days that we could just add a flag to ArrowReaderOptions 🤔 . I agree the idea of Required for the metadata reader doesn't really make a lot of sense.

The origin story is that back in the day, the column index had a NONE placeholder variant, so a missing column index was replaced with that. The offset index had no such placeholder, as the belief was all offset indexes would be populated (which makes sense, you can't do page pruning without them). The offset index parser simply panic'd if an individual cell was missing. To get rid of the panic, the policy was introduced; if the policy was Required, then an error was returned if an index was missing, if Optional then the entire offset index would be thrown away and decoding stopped. Now that individual cells of the offset index can be None, there's less need for Required. I think we should at least change the policy to Optional when the old with_page_index(true) is used.

I recommend documenting what the [u32] means and the invariants. I think it is something like "sorted indexes"

I was somewhat confused on first read if it was using bitmaps; It is fine that it isn't, but if we want to get fancy / need more space efficiency here in the future we could switch to bitmaps

I'll look at the ColumnChunkMask docs and see what needs better clarification. I don't know that usize vs u32 matters so much here, but it does matter more when we use a similar construct in the Grid storage (#11159). No need to store the wider type in the metadata when parquet itself doesn't support array indices greater than 2^31 - 1

ok, fixed that up some in b920667

It may also be worth mentioning this is cheap to clone (some Arcs)

While we're on cloning, if you have a moment to opine on #11297 I'd appreciate your thoughts.

is it worth making this API return Result and checking that the column index mask doesn't refer to out of bounds columns (e.g. some column index greater than the number of columns?)