This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 4cc296bca1 fix: Avoid incorrect min/max stats for BYTE_ARRAY decimals
of unequal byte lengths (#10861)
4cc296bca1 is described below
commit 4cc296bca10303382989ec7cebc98522e3230ddb
Author: Neil Conway <[email protected]>
AuthorDate: Thu Aug 27 14:59:52 2026 -0400
fix: Avoid incorrect min/max stats for BYTE_ARRAY decimals of unequal byte
lengths (#10861)
# Which issue does this PR close?
- Closes #10860.
# Rationale for this change
`compare_greater_byte_array_decimals` sign-extends the shorter value
when comparing two's-complement byte arrays of different lengths. When
the longer value's extra leading bytes were all sign-extension bytes,
the final comparison dropped exactly one byte from each side and then
lexicographically compares the remaining byte slices. This is incorrect
when the byte slices are of different lengths.
This can result in exact row-group/page stats with incorrect values
(e.g., swapped min and max).
Align the tails by skipping the length difference on the longer side
only, matching parquet-mr.
# What changes are included in this PR?
* Fix bug in byte array comparison
* Add tests
# Are these changes tested?
Yes; new tests added.
# Are there any user-facing changes?
No.
# AI usage
Bug found and fix written with Claude Code Fable 5; I reviewed and
understand the resulting code.
---
parquet/src/column/writer/mod.rs | 88 +++++++++++++++++++++++++++++++++++++---
1 file changed, 83 insertions(+), 5 deletions(-)
diff --git a/parquet/src/column/writer/mod.rs b/parquet/src/column/writer/mod.rs
index 6ef18419f2..8b6f33cc35 100644
--- a/parquet/src/column/writer/mod.rs
+++ b/parquet/src/column/writer/mod.rs
@@ -1935,10 +1935,11 @@ fn compare_greater_byte_array_decimals(a: &[u8], b:
&[u8]) -> bool {
return (first_a as i8) > (first_b as i8);
}
- // When the lengths are unequal and the numbers are of the same
- // sign we need to do comparison by sign extending the shorter
- // value first, and once we get to equal sized arrays, lexicographical
- // unsigned comparison of everything but the first byte is sufficient.
+ // When the lengths are unequal and the numbers are of the same sign,
+ // sign-extend the shorter value: if any of the longer value's extra
+ // leading bytes differs from the sign-extension byte it has the larger
+ // magnitude, and otherwise those bytes are redundant and the aligned
+ // equal-length tails decide via unsigned lexicographical comparison.
let extension: u8 = if (first_a as i8) < 0 { 0xFF } else { 0 };
@@ -1958,7 +1959,8 @@ fn compare_greater_byte_array_decimals(a: &[u8], b:
&[u8]) -> bool {
}
}
- (a[1..]) > (b[1..])
+ let tail_length = a_length.min(b_length);
+ (a[a_length - tail_length..]) > (b[b_length - tail_length..])
}
/// Truncate a UTF-8 slice to the longest prefix that is still a valid UTF-8
string,
@@ -2591,6 +2593,42 @@ mod tests {
}
}
+ #[test]
+ fn test_column_writer_byte_array_min_max_unequal_lengths() {
+ // Byte-array decimal min/max with values of different encoded lengths
+ // https://github.com/apache/arrow-rs/issues/10860
+ let page_writer = get_test_page_writer();
+ let props = Default::default();
+ let mut writer =
get_test_decimals_column_writer::<ByteArrayType>(page_writer, 0, 0, props);
+ writer
+ .write_batch(
+ &[
+ ByteArray::from(vec![0u8, 255u8]), // 255
+ ByteArray::from(vec![0u8, 128u8, 0u8]), // 32768
+ ByteArray::from(vec![255u8, 127u8]), // -129
+ ByteArray::from(vec![128u8]), // -128
+ ],
+ None,
+ None,
+ )
+ .unwrap();
+ let metadata = writer.close().unwrap().metadata;
+ let stats = metadata.statistics().expect("metadata missing
statistics");
+ let Statistics::ByteArray(stats) = stats else {
+ panic!("expecting Statistics::ByteArray");
+ };
+ // -129
+ assert_eq!(
+ stats.min_opt().unwrap(),
+ &ByteArray::from(vec![255u8, 127u8])
+ );
+ // 32768
+ assert_eq!(
+ stats.max_opt().unwrap(),
+ &ByteArray::from(vec![0u8, 128u8, 0u8])
+ );
+ }
+
#[test]
fn test_column_writer_uint32_converted_type_min_max() {
let page_writer = get_test_page_writer();
@@ -4032,6 +4070,46 @@ mod tests {
&[0u8,],
&[255u8, 35u8, 0u8, 0u8,],
),);
+
+ // Unequal lengths where the longer value's extra leading bytes are all
+ // sign extension, so the aligned tails decide.
+ // https://github.com/apache/arrow-rs/issues/10860
+
+ // 32768 > 255
+ assert!(compare_greater_byte_array_decimals(
+ &[0u8, 128u8, 0u8,],
+ &[0u8, 255u8,],
+ ),);
+ assert!(!compare_greater_byte_array_decimals(
+ &[0u8, 255u8,],
+ &[0u8, 128u8, 0u8,],
+ ),);
+ // -128 > -129
+ assert!(compare_greater_byte_array_decimals(
+ &[128u8,],
+ &[255u8, 127u8,],
+ ),);
+ assert!(!compare_greater_byte_array_decimals(
+ &[255u8, 127u8,],
+ &[128u8,],
+ ),);
+ // -128 > -256
+ assert!(compare_greater_byte_array_decimals(
+ &[128u8,],
+ &[255u8, 0u8,],
+ ),);
+ // 10 (with a redundant leading zero) > 5
+ assert!(compare_greater_byte_array_decimals(&[0u8, 10u8,], &[5u8,],),);
+ assert!(compare_greater_byte_array_decimals(&[10u8,], &[0u8, 5u8,],),);
+ // equal values of different lengths are not greater in either
direction
+ assert!(!compare_greater_byte_array_decimals(
+ &[255u8, 128u8,],
+ &[128u8,],
+ ),);
+ assert!(!compare_greater_byte_array_decimals(
+ &[128u8,],
+ &[255u8, 128u8,],
+ ),);
}
#[test]
