[thrift-remodel] Optimize `convert_row_groups`

[alamb]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
I ran into this while working on the benchmark here

• Measure best case performance for the new thrift-remodel / custom thrift parser #8441

I noticed a substantial amount of time (15% of the overall time) in the benchmark was spent in convert_row_groups

arrow-rs/parquet/src/file/metadata/thrift_gen.rs

Lines 247 to 257 in b4b4d26

	fn convert_row_groups(
	mut row_groups: Vec<RowGroup>,
	schema_descr: Arc<SchemaDescriptor>,
	) -> Result<Vec<RowGroupMetaData>> {
	let mut res: Vec<RowGroupMetaData> = Vec::with_capacity(row_groups.len());
	for rg in row_groups.drain(0..) {
	res.push(convert_row_group(rg, schema_descr.clone())?);
	}
	Ok(res)
	}

Describe the solution you'd like
I think that code could likely be optimized.

Describe alternatives you've considered
Two obvious candidates:

1. Use the into_iter() / collect pattern to map the results (which is highly optimized in Rust)
2. Don't clone the Arc<SchemaDescriptor> -- I think it only needs a reference

Another thing would be to decode directly into RowGroupMetaData somehow (maybe make RowGRoupMetaData a view on an inner RowGroup 🤔

struct RowGroupMetaData {
  inner: RowGroup
}

Additional context

[etseidl]

Another thing would be to decode directly into RowGroupMetaData somehow (maybe make RowGRoupMetaData a view on an inner RowGroup 🤔

I did this exercise very early on and found almost no improvement over decoding to thrift objects and then pulling them apart. But I've learned a lot since then so it's probably worth reexamining this.

[etseidl]

Thinking a bit more about this, I think we can rework the row group decoding a bit to not double allocate the vectors used to hold column chunks and row groups. I'll run that to ground tomorrow.

[alamb]

Thinking a bit more about this, I think we can rework the row group decoding a bit to not double allocate the vectors used to hold column chunks and row groups. I'll run that to ground tomorrow.

amazing!

[etseidl]

I've just finished a first cut at eliminating the convert_ functions, and so far so good. It's take about 10% off the decode time (I indeed must have learned something over the last few months). The good news is this is work that would need to be done anyway to support skipping fields, etc. And would not be breaking.

The bad news (for now) is I'll need to see how hard it is to wrap in encryption support without two copies of everything.

[etseidl]

Only good news now. I've removed some of the worst convert_ functions with and without encryption support. Benchmarks look good.

group                             gh5854                                 no_conv
-----                             ------                                 -------
decode parquet metadata           1.08     19.5±0.37µs        ? ?/sec    1.00     18.1±0.35µs        ? ?/sec
decode parquet metadata (wide)    1.14     90.7±3.11ms        ? ?/sec    1.00     79.6±1.52ms        ? ?/sec
open(default)                     1.13     21.2±0.64µs        ? ?/sec    1.00     18.8±0.52µs        ? ?/sec
open(page index)                  1.00    257.5±5.27µs        ? ?/sec    1.00    257.0±4.10µs        ? ?/sec

I have noticed, however, that there is a regression due to the encryption, even for calls that are encryption unaware (the "decode parquet metadata" benches).

group                             no_conv                                no_conv_no_encr
-----                             -------                                ---------------
decode parquet metadata           1.04     18.1±0.35µs        ? ?/sec    1.00     17.4±0.72µs        ? ?/sec
decode parquet metadata (wide)    1.07     79.6±1.52ms        ? ?/sec    1.00     74.3±1.67ms        ? ?/sec
open(default)                     1.04     18.8±0.52µs        ? ?/sec    1.00     18.0±0.31µs        ? ?/sec
open(page index)                  1.01    257.0±4.10µs        ? ?/sec    1.00    255.1±5.80µs        ? ?/sec

[alamb]

label_issue.py automatically added labels {'parquet'} from #8574