alamb commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4517169221
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
Review Comment:
what extra coverage does this test add?
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -72,10 +72,36 @@ fn join_streams_variable(
}
}
+/// Returns the number of values physically present in a BYTE_STREAM_SPLIT
page.
+///
+/// `num_values` is an upper bound rather than an exact count: for a V1 data
page it is
+/// the number of levels, which includes nulls (and repetition levels), while
only
+/// non-null values are encoded. The byte streams therefore must not hold more
values
+/// than `num_values`, but they may legitimately hold fewer. The stride
between the
+/// streams, and so the number of values that can be decoded, comes from the
encoded
+/// length. If the page declares more values than are present, the column
reader reports
+/// that it could not read enough values instead of indexing past the end of
the buffer.
+fn check_encoded_value_count(encoded_values: usize, num_values: usize) ->
Result<usize> {
+ if encoded_values > num_values {
+ return Err(general_err!(
+ "Invalid BYTE_STREAM_SPLIT data: {encoded_values} encoded values
exceeds the {num_values} values expected"
+ ));
+ }
+ Ok(encoded_values)
+}
+
impl<T: DataType> Decoder<T> for ByteStreamSplitDecoder<T> {
fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ let type_size = T::get_type_size();
+ if !data.len().is_multiple_of(type_size) {
+ return Err(general_err!(
+ "Invalid BYTE_STREAM_SPLIT data length {} for values of size
{type_size}",
+ data.len()
+ ));
+ }
+
+ self.total_num_values = check_encoded_value_count(data.len() /
type_size, num_values)?;
Review Comment:
👍 this is looking really nice
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ for count in [8, 63, usize::MAX] {
+ decoder.set_data(Bytes::from(vec![0; 64]), count).unwrap();
+ assert_eq!(decoder.values_left(), 8);
+ let mut values = vec![0.; 63];
+ assert_eq!(decoder.get(&mut values[..3]).unwrap(), 3);
+ assert_eq!(decoder.skip(2).unwrap(), 2);
+ assert_eq!(decoder.get(&mut values).unwrap(), 3);
+ assert_eq!(decoder.values_left(), 0);
+ assert_eq!(decoder.get(&mut values).unwrap(), 0);
+ }
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_invalid_data_length() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ let err = decoder.set_data(Bytes::from(vec![0; 63]), 8).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data length 63 for
values of size 8"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_more_values_than_expected() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+ let err = decoder.set_data(Bytes::from(vec![0; 64]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values
exceeds the 7 values expected"
+ );
+
+ let mut decoder =
VariableWidthByteStreamSplitDecoder::<FixedLenByteArrayType>::new(5);
+ let err = decoder.set_data(Bytes::from(vec![0; 40]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values
exceeds the 7 values expected"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_flba_uses_encoded_value_count() {
Review Comment:
likewise what extra coverage does this chekc?
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -136,6 +136,19 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_arrow_rs_gh_11136_byte_stream_split_truncated_page() {
+ // Checked-in malformed page: declared value count exceeds
BYTE_STREAM_SPLIT payload.
+ let file =
Bytes::from_static(include_bytes!("ARROW-RS-GH-11136-bss-truncated.parquet"));
+ let reader = ArrowReaderBuilder::try_new(file).unwrap().build().unwrap();
+ let err = reader.collect::<Result<Vec<_>, _>>().unwrap_err();
+ assert!(
+ err.to_string()
+ .contains("insufficient values read from column"),
Review Comment:
Why isn't this the new error?
--
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]
