TODO from original PR

this duplicates a lot, not sure if anything can be done here

the name used here is not aligned with what other convert_ functions do

I guess this doesn't hurt, but it shouldn't be necessary. The row groups already passed through the OrdinalAssigner in parquet_metadata_from_bytes, and row_group_from_encrypted_thrift re-uses the ordinal from the RowGroupMetaData passed in.

I reverted the change here.

I wonder if first_has_ordinal should be an option, and then set it if it's None. If/when we implement row group selection the first ordinal seen may not be 0.

But then enumeration won't work at all, and we'd have to rely on ordinal in the metadata? Actually, row numbers feature won't work at all, since it relies on having information (sizes in rows) about all row groups to figure out first row index of the row group it reads. Unless some trick that I'm not aware of is used.

Note that row numbers have to be stable across queries (i.e. independent of whether there was filtering), otherwise we would've implemented them on the client side by just enumerating rows.

Yes, but it seems this code is executed regardless of the presence of a virtual column. It looks to me like skipping the first rowgroup will result in an error if ordinals are present.

This code should only error out if either ordinal is present in metadata for some row group but not for others, or vice-versa. That shouldn't happen with row group skipping, i.e. even if we skip, all the remaining row groups that we iterate through would fulfil this condition, right?

What I'd like though, is that the build_row_number_reader fails if skipping occurs. Or that it ensures that there's no row skipping if it's invoked. Not sure how to ensure that at this point though.

If I skip row groups 0 and 1 and go straight to row group 2 (think externally cached statistics with point look up), which has ordinal set to 2 in the footer, self.first_has_ordinal is false and rg_has_ordinal is true. L904 will evaluate false, L906 will evaluate true and return an error.

I'll take a closer look at the rest of this PR to see if there's a way to make skipping and row numbers mutually exclusive.

I see why we think differently. I was assuming then that the row group 2 would be the one to set self.first_has_ordinal. Because for example the loop that invokes ensure would be going for ordinal in 0..list_ident.size, whe size is going to be based only on the row groups after skipping.

Is that not going to happen?
Perhaps there might need some changes to be made once row group skipping is implemented, not sure how best to guard against getting the intended behaviour broken without catching it.

Because for example the loop that invokes ensure would be going for ordinal in 0..list_ident.size, whe size is going to be based only on the row groups after skipping.

Ah, but here we're in a function whose input is the entire encoded footer. list_ident.size will always be the unfiltered number of row groups. So I think we'll either:

• loop over ordinal in 0..list_ident.size, but skip decoding if ordinal not in a list
• assuming an index, loop over a list of ordinals, decode the row group using a range from the index, and pass ordinal as actual_ordinal

In either event, we'd pass 2 as actual_ordinal in my scenario above. But yeah, that's pretty far down the road. Guess we can solve it then.

Now I see why you proposed to use Option. I pushed a change for that.

@vustef I think this is where we'd be able to detect row group filtering. There would be a with_row_group_selection() or some such function added to control skipping, and a check could be added both here and in the new function to disallow setting both.

Thanks for figuring this out. I guess there's no action that we can take right now then, please let me know if it's otherwise.

I don't understand this part? The row groups were supplied in some particular order (by the row_groups iterator), and we're reordering by row group ordinal instead? Wouldn't that cause row number mismatches with other columns that continue reading in the original order? It seems like we actually need:

let selected_ordinals = HashMap<i16, usize> = row_groups
    .enumerate()
    .map(...)
    .collect::<Result<_>>()?;

and then ranges needs to use that enumeration ordinal (not the row group ordinal):

if let Some(i) = selected_ordinals.get(&ordinal) {
    ranges.push((i, ...);
}

... so that the sorted ranges match the original row_group iterator's order?

I messed this up when I started computing first row indexes...thank you for catching this. WIll follow up shortly with tests and the fix.

not a fan of panics, but if we're going to panic why not just

assert!(
    is_virtual_column(field), 
    "...", 
    field.name()
);

Me neither, but seemed like a unwritten rule that all these with_ methods in ArrowReaderOptions return Self rather than a Result. Please comment in the new PR if I should change that behaviour.

@alamb also checking for your opinion on this.

Yes, the builder-like with_ functions don't give much opportunity for validity checking. We could probably use a proper ArrowReaderOptionsBuilder and do that kind of checking in build(). Or just change this one to a setter (so it's obvious it can't be chained) and return a Result<()>.

I think it is ok to return errors when trying to build options, like

let options = options.with_virtual_columns(cols)?;

If there is an error it is unlikely the code wants to continue configuring the options anyways