alamb commented on code in PR #9794:
URL: https://github.com/apache/arrow-rs/pull/9794#discussion_r3173175887
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -147,11 +148,38 @@ fn read_file(name: &str) -> Result<usize, ParquetError> {
Ok(num_rows)
}
+#[test]
+fn non_standard_delta_blocks() {
+ let file = Bytes::from_static(include_bytes!("bigdelta.parquet"));
+ use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
+
+ let selectors = vec![RowSelector::skip(1000), RowSelector::select(5)];
+
+ let selection: RowSelection = selectors.into();
+ let reader = ArrowReaderBuilder::try_new(file)
+ .unwrap()
+ .with_row_selection(selection)
+ .build()
+ .unwrap();
+
+ if let Some(maybe_batch) = reader.into_iter().next() {
+ // TODO: uncomment if we ever allow skipping miniblocks > 64 elements
+ //let batch = maybe_batch.expect("skip should succeed");
+ //assert_eq!(batch.num_rows(), 5);
+ assert!(maybe_batch.is_err());
Review Comment:
technically this check for `is_err` is redundant as `unwrap_err` will panic
if maybe_batch is Ok
##########
parquet/src/encodings/decoding.rs:
##########
@@ -847,10 +847,21 @@ where
self.values_left -= 1;
}
- let mini_block_batch_size = match T::T::PHYSICAL_TYPE {
- Type::INT32 => 32,
- Type::INT64 => 64,
- _ => unreachable!(),
+ // 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 {
Review Comment:
an error certainly seems better than a panic
--
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]
