etseidl commented on code in PR #11092:
URL: https://github.com/apache/arrow-rs/pull/11092#discussion_r4022321992
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -800,12 +823,30 @@ fn count_within_budget_offsets<T: ByteArrayType>(
n
}
+/// Returns `true` if `a > b`.
+///
+/// `BYTE_ARRAY` columns logically typed as `DECIMAL` store values as
+/// two's-complement, big-endian bytes, so they must be compared with
+/// [`compare_greater_byte_array_decimals`] rather than plain unsigned
+/// byte-wise `Ord`, or negative values would sort as the largest values.
+/// This mirrors the comparator `compare_greater` uses in the non-Arrow
+/// column writer path (`crate::column::writer`).
Review Comment:
```suggestion
```
Again, I think this level of explanation is unnecessary
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -432,6 +433,15 @@ pub struct ByteArrayEncoder {
bloom_filter: Option<Sbbf>,
bloom_filter_target_fpp: f64,
geo_stats_accumulator: Option<Box<dyn GeoStatsAccumulator>>,
+ /// Whether this column is a `BYTE_ARRAY` logically typed as `DECIMAL`.
+ ///
+ /// Decimal values stored as `BYTE_ARRAY` use two's-complement, big-endian
+ /// encoding, so plain unsigned byte-wise comparison (used for min/max
+ /// statistics on every other `BYTE_ARRAY` column) gives the wrong
+ /// ordering for negative values. When this is set, statistics use
+ /// [`compare_greater_byte_array_decimals`] instead, matching the
+ /// comparator used by the non-Arrow column writer path.
Review Comment:
```suggestion
```
I think the field name is pretty self explanatory.
##########
parquet/src/column/writer/mod.rs:
##########
@@ -2001,8 +2000,25 @@ fn compare_greater_f16(a: &[u8], b: &[u8]) -> bool {
a.total_cmp(&b) == Ordering::Greater
}
+/// Returns `true` if the column described by `basic_type_info` is a Decimal
column
+/// (either via `ConvertedType::DECIMAL` or `LogicalType::Decimal`),
regardless of
+/// whether its physical type is `BYTE_ARRAY` or `FIXED_LEN_BYTE_ARRAY`.
+///
+/// Decimal values stored as (FIXED_LEN_)BYTE_ARRAY use two's-complement,
big-endian
+/// signed-integer encoding, which sorts differently from the unsigned,
byte-wise
+/// lexicographic order used for other BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY data.
Callers
+/// that need unsigned byte-wise comparisons or increments (e.g. statistics
+/// truncation) must special-case or skip Decimal columns.
Review Comment:
```suggestion
/// Returns `true` if the column described by `basic_type_info` is a Decimal
column
```
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1352,14 +1352,17 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a,
E> {
// from that of FIXED_LEN_BYTE_ARRAY sort order.
// So truncation of those types could lead to inaccurate min/max
statistics
Type::FIXED_LEN_BYTE_ARRAY
- if !matches!(
- self.descr.logical_type_ref(),
- Some(&LogicalType::Decimal { .. } | &LogicalType::Float16)
- ) =>
+ if !is_decimal_descr(self.descr.get_basic_info())
+ && !matches!(self.descr.logical_type_ref(),
Some(&LogicalType::Float16)) =>
{
true
}
- Type::BYTE_ARRAY => true,
+ // Decimal values encoded as BYTE_ARRAY use two's-complement,
signed
+ // big-endian comparison, which differs from the unsigned,
byte-wise
+ // comparison used to truncate/increment other BYTE_ARRAY values.
+ // Truncating such a value could produce an incorrect min/max, so
skip
+ // truncation for Decimal BYTE_ARRAY columns as well.
Review Comment:
```suggestion
// As with FIXED_LEN_BYTE_ARRAY, do not truncate Decimal values
```
##########
parquet/src/column/writer/mod.rs:
##########
@@ -4568,6 +4584,58 @@ mod tests {
}
}
+ #[test]
+ fn test_decimal_byte_array_min_max_no_statistics_truncation() {
+ // Regression test for the truncation path re-introducing the unsigned
vs.
+ // signed two's-complement comparison bug fixed for
apache/arrow-rs#11073:
+ // `truncate_statistics` must not byte-wise truncate/increment
+ // `Statistics::ByteArray` min/max for a Decimal-typed BYTE_ARRAY
column,
+ // even when `statistics_truncate_length` is configured and the encoded
+ // value is longer than the truncate length.
Review Comment:
```suggestion
// See https://github.com/apache/arrow-rs/issues/11073
```
##########
parquet/src/column/writer/mod.rs:
##########
@@ -2001,8 +2000,25 @@ fn compare_greater_f16(a: &[u8], b: &[u8]) -> bool {
a.total_cmp(&b) == Ordering::Greater
}
+/// Returns `true` if the column described by `basic_type_info` is a Decimal
column
+/// (either via `ConvertedType::DECIMAL` or `LogicalType::Decimal`),
regardless of
+/// whether its physical type is `BYTE_ARRAY` or `FIXED_LEN_BYTE_ARRAY`.
+///
+/// Decimal values stored as (FIXED_LEN_)BYTE_ARRAY use two's-complement,
big-endian
+/// signed-integer encoding, which sorts differently from the unsigned,
byte-wise
+/// lexicographic order used for other BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY data.
Callers
+/// that need unsigned byte-wise comparisons or increments (e.g. statistics
+/// truncation) must special-case or skip Decimal columns.
+pub(crate) fn is_decimal_descr(basic_type_info: &BasicTypeInfo) -> bool {
Review Comment:
perhaps inline this
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -6565,4 +6565,110 @@ mod tests {
);
assert_eq!(parquet_schema.column(1).path().string(), "row.b");
}
+
+ /// Regression test for https://github.com/apache/arrow-rs/issues/11073
+ ///
+ /// Decimal values backed by `BYTE_ARRAY`/`FIXED_LEN_BYTE_ARRAY` are
+ /// stored as two's-complement, big-endian bytes. Statistics for such a
+ /// column must therefore be compared with sign-awareness rather than
+ /// plain unsigned byte-wise `Ord`, or negative values (whose leading
+ /// byte has the sign bit set) sort as the largest values.
+ ///
+ /// This checks that the `ArrowColumnWriter` path (going through
+ /// `byte_array.rs`'s `ByteArrayEncoder`) produces the same min/max
+ /// statistics as writing the column directly with the low-level
+ /// `SerializedFileWriter` API, for both `BinaryArray` (the buggy case)
+ /// and `FixedSizeBinaryArray` (which already worked correctly), using
+ /// the exact 1-byte-decimal repro from the issue: values -1, 0, 1.
Review Comment:
```suggestion
/// This checks that the `ArrowColumnWriter` path produces the same
min/max
/// statistics as writing the column directly with the low-level
/// `SerializedFileWriter` API, for both `BinaryArray` (the buggy case)
/// and `FixedSizeBinaryArray` (which already worked correctly).
```
--
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]
