This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11484-cf77edaa8122b9047bbe67def2adcf71240703e7 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit a9b8d27da30e52c778c10f2a76916a0af789fc00 Author: Adrian Garcia Badaracco <[email protected]> AuthorDate: Sat Oct 10 07:42:01 2026 +0000 fix(parquet): return an error, not a panic, when an offset index page goes past its column chunk (#11484) # Which issue does this PR close? - Part of https://github.com/apache/arrow-rs/issues/11234. # Rationale for this change If the offset index gives a page size that goes past the end of the column chunk, the async reader and the push decoder panic in `Bytes::slice`. A reader must return an error for a malformed file, not panic. ```rust // offset index of column 0: the last page is 1 byte too long let mut decoder = ParquetPushDecoderBuilder::try_new_decoder(metadata)?.build()?; // ... push the requested ranges ... decoder.try_decode() // panic: range end out of bounds: 127 <= 126 ``` This PR is split out of https://github.com/apache/arrow-rs/pull/11238 to make that PR smaller. # What changes are included in this PR? `ColumnChunkData::get_bytes` checks the length before it slices: ```diff fn get_bytes(&self, start: u64, length: usize) -> Result<Bytes> { - Ok(self.get(start)?.slice(..length)) + let data = self.get(start)?; + if data.len() < length { + return Err(general_err!("... has {} bytes, expected {length}")); + } + Ok(data.slice(..length)) } ``` This PR does not change `ColumnChunkData::get`, which can still panic if the start offset is outside the column chunk. # Are these changes tested? Yes, with two new tests: `test_decoder_offset_index_page_past_column_chunk` (through the push decoder) and `get_bytes_errors_on_short_data` (unit test). - **Before:** `panicked at bytes-1.12.1/src/bytes.rs:374:28: range end out of bounds: 127 <= 126` - **After:** `Parquet error: column chunk data at offset ... has 126 bytes, expected 127` # Are there any user-facing changes? A read of a file with such an offset index returns an error. Before, it caused a panic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]> --- parquet/src/arrow/in_memory_row_group.rs | 28 +++++++++++++++++++++- parquet/src/arrow/push_decoder/mod.rs | 41 ++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/parquet/src/arrow/in_memory_row_group.rs b/parquet/src/arrow/in_memory_row_group.rs index d75a81efbf..45399b0946 100644 --- a/parquet/src/arrow/in_memory_row_group.rs +++ b/parquet/src/arrow/in_memory_row_group.rs @@ -405,7 +405,14 @@ impl ChunkReader for ColumnChunkData { } fn get_bytes(&self, start: u64, length: usize) -> crate::errors::Result<Bytes> { - Ok(self.get(start)?.slice(..length)) + let data = self.get(start)?; + if data.len() < length { + return Err(general_err!( + "column chunk data at offset {start} has {} bytes, expected {length}", + data.len() + )); + } + Ok(data.slice(..length)) } } @@ -423,3 +430,22 @@ impl Iterator for ColumnChunkIterator { } impl PageIterator for ColumnChunkIterator {} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn get_bytes_errors_on_short_data() { + let dense = ColumnChunkData::Dense { + offset: 100, + data: Bytes::from_static(b"0123456789"), + }; + assert_eq!( + dense.get_bytes(105, 5).unwrap(), + Bytes::from_static(b"56789") + ); + let err = dense.get_bytes(105, 6).unwrap_err().to_string(); + assert!(err.contains("has 5 bytes, expected 6"), "{err}"); + } +} diff --git a/parquet/src/arrow/push_decoder/mod.rs b/parquet/src/arrow/push_decoder/mod.rs index 94bc0f9eba..82dc037351 100644 --- a/parquet/src/arrow/push_decoder/mod.rs +++ b/parquet/src/arrow/push_decoder/mod.rs @@ -1027,6 +1027,7 @@ mod test { }; use crate::arrow::{ArrowWriter, ProjectionMask}; use crate::errors::ParquetError; + use crate::file::metadata::page_index::PageIndexBuilder; use crate::file::metadata::{PageIndexPolicy, ParquetMetaDataPushDecoder}; use crate::file::properties::WriterProperties; use arrow::compute::kernels::cmp::{gt, lt}; @@ -3125,6 +3126,46 @@ mod test { Arc::new(metadata) } + /// An offset index whose last page is longer than the column chunk gives + /// an error, not a panic. + #[test] + fn test_decoder_offset_index_page_past_column_chunk() { + let metadata = Arc::unwrap_or_clone(test_file_parquet_metadata_with_offset_index()); + let num_columns = metadata.row_group(0).num_columns(); + let mut page_index = PageIndexBuilder::new(metadata.num_row_groups(), num_columns); + for row_group in 0..metadata.num_row_groups() { + let row_group_index = metadata.page_index_for_row_group(row_group); + for column in 0..num_columns { + let mut offset_index = row_group_index.offset_index(column).unwrap().clone(); + if (row_group, column) == (0, 0) { + let last = offset_index.page_locations.last_mut().unwrap(); + last.compressed_page_size += 1; + } + page_index.put_offset_index(offset_index, row_group, column); + } + } + let metadata = metadata + .into_builder() + .set_page_index(Some(Arc::new(page_index.build()))) + .build(); + + let mut decoder = ParquetPushDecoderBuilder::try_new_decoder(Arc::new(metadata)) + .unwrap() + .with_row_groups(vec![0]) + .build() + .unwrap(); + let ranges = expect_needs_data(decoder.try_decode()); + push_ranges_to_decoder(&mut decoder, ranges); + let err = loop { + match decoder.try_decode() { + Ok(DecodeResult::Data(_)) => {} + Ok(other) => panic!("expected an error, got {other:?}"), + Err(e) => break e.to_string(), + } + }; + assert!(err.contains("has 126 bytes, expected 127"), "{err}"); + } + /// return the metadata for the test file, including the offset index fn test_file_parquet_metadata_with_offset_index() -> Arc<ParquetMetaData> { let mut metadata_decoder = ParquetMetaDataPushDecoder::try_new(test_file_len())
