etseidl commented on code in PR #11021:
URL: https://github.com/apache/arrow-rs/pull/11021#discussion_r3963432821
##########
parquet/src/encodings/decoding.rs:
##########
@@ -852,23 +857,13 @@ where
}
// See https://github.com/apache/arrow-rs/pull/9794.
- // The parquet spec actually allows for miniblock sizes other than 32
or 64, but
- // no current writers use anything else. Using values_per_mini_block
directly
- // for the skip_buffer doesn't allow stack allocation and leads to a
significant
- // drop in performance. We'll settle for erroring out here and come up
with a
- // better fix if writers ever start getting creative with block sizes.
- let mini_block_batch_size = match self.values_per_mini_block {
- 32 => 32,
- 64 => 64,
- _ => {
- return Err(general_err!(
- "cannot skip miniblock of size {}",
- self.values_per_mini_block
- ));
- }
- };
-
- let mut skip_buffer = vec![T::T::default(); mini_block_batch_size];
+ // The parquet spec allows miniblock sizes other than the 32 and 64
this crate
+ // writes, and sizing skip_buffer off values_per_mini_block would mean
a large
+ // allocation on pages written with big miniblocks. The buffer only
exists to
+ // walk last_value forward, so keep it at the size the common cases
need and
+ // consume wider miniblocks a chunk at a time.
+ let mut skip_buffer =
+ vec![T::T::default();
self.values_per_mini_block.min(MAX_SKIP_BUFFER_VALUES)];
Review Comment:
My bad 😅. Didn't see the extra space. We'll see how the benches look now.
Thanks!
--
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]
