jaideeppyne commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4514998393
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -149,6 +155,56 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_byte_stream_split_truncated_page() {
Review Comment:
Done in 0e75fbd. The 487 byte file is now checked in as
`parquet/tests/arrow_reader/ARROW-RS-GH-11136-bss-truncated.parquet` (byte for
byte the reproducer linked in the description) and the test reads it with
`include_bytes!`, the same way `bigdelta.parquet` is used. The writer and
header patching code is gone.
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -684,8 +684,49 @@ impl RepetitionLevelDecoder for RepetitionLevelDecoderImpl
{
#[cfg(test)]
mod tests {
use super::*;
+ use crate::data_type::DoubleType;
use crate::encodings::rle::RleEncoder;
+ use crate::schema::types::{ColumnDescriptor, ColumnPath, Type as
SchemaType};
use rand::{prelude::*, rng};
+ use std::sync::Arc;
+
+ #[test]
+ fn test_byte_stream_split_v1_nulls_use_encoded_len() {
Review Comment:
Not enough to justify it, so I removed it in 0e75fbd.
The case it pinned is a V1 nullable page where the level count is larger
than the number of encoded values. That is already covered end to end: when I
temporarily made `set_data` require `num_values == data.len() / type_size`,
`arrow::arrow_reader::tests::column_reader::test_primitive_single_column_reader_test`
and `arrow::arrow_writer::tests::statistics_null_counts_only_nulls` both
failed. The corrupt count case is covered by the checked-in file.
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -86,9 +86,18 @@ fn join_streams_variable(
}
impl<T: DataType> Decoder<T> for ByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
Good point, ignoring it entirely was too loose. In 0e75fbd both decoders now
check it as an upper bound: `set_data` returns an error if the page holds more
encoded values than `num_values`.
It can't be strict equality. `ColumnValueDecoderImpl::set_data` passes
`num_values.unwrap_or(num_levels)`. For V2 pages that is the non-null count,
but for V1 pages it is the level count, which includes nulls (and for repeated
columns every level), while only non-null values are encoded. With an equality
check, reading ordinary nullable V1 pages fails; the two existing tests
mentioned in the other thread fail that way.
The other direction, fewer encoded values than `num_values`, is the #11121
case. Since `values_left()` now comes from the encoded length, `get` and `skip`
stop at the end of the streams and the column reader returns `insufficient
values read from column`, instead of `join_streams_const` indexing past the
buffer. That is also why a separate length check in `get` (the other option
@jhorstmann mentioned) isn't needed.
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -164,7 +173,7 @@ impl<T: DataType> VariableWidthByteStreamSplitDecoder<T> {
}
impl<T: DataType> Decoder<T> for VariableWidthByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
Same change here: `VariableWidthByteStreamSplitDecoder::set_data` uses the
same upper bound check through a shared `check_encoded_value_count` helper, and
`test_byte_stream_split_rejects_more_values_than_expected` covers both decoders.
--
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]
