Fix incorrect Decimal statistics for BYTE_ARRAY columns written via ArrowColumnWriter - #11092
Open
DeviousCardi wants to merge 3 commits into
Open
DeviousCardi wants to merge 3 commits into
DeviousCardi wants to merge 3 commits into
Conversation
…rrowColumnWriter The Arrow-facing byte array encoder (parquet::arrow::arrow_writer::byte_array) computed min/max statistics with plain unsigned byte-wise `Ord` comparison, regardless of logical type. Decimal values stored as BYTE_ARRAY use two's-complement, big-endian encoding, so negative values (whose leading byte has the sign bit set) were incorrectly treated as the largest values. The low-level SerializedFileWriter path already handled this correctly via compare_greater_byte_array_decimals in column::writer, and so did FixedSizeBinaryArray (routed through the generic column writer instead of this encoder) - only the Arrow BinaryArray/StringArray-as-Decimal path via ArrowColumnWriter was affected. Widen compare_greater_byte_array_decimals to pub(crate) and reuse it in ByteArrayEncoder: try_new now records whether the column's logical/converted type is Decimal (available from the ColumnDescPtr the encoder is already constructed with), and encode()/compute_min_max() use that flag to pick a sign-aware comparator instead of duplicating the two's-complement logic. Adds a regression test porting the issue's repro, checking that the direct Parquet, Arrow FixedSizeBinaryArray, and Arrow BinaryArray write paths all agree on min/max statistics for a decimal column. Fixes apache#11073. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion logic (apache#11073 follow-up) Code review of the apache#11073 fix found that truncate_statistics() still truncated/incremented Statistics::ByteArray min/max using unsigned byte-wise logic unconditionally whenever statistics_truncate_length was set, reintroducing the same signed-vs-unsigned comparison bug inside the truncation path for BYTE_ARRAY-encoded Decimal columns whose two's complement value exceeds the truncate length. The FixedLenByteArray arm already guarded against this via can_truncate_value(); extend can_truncate_value() to also exclude Decimal BYTE_ARRAY columns and have the ByteArray arm use it, mirroring the FixedLenByteArray arm. Also extract the duplicated decimal-detection predicate (converted_type == DECIMAL || logical_type is Decimal) - previously copy-pasted in both byte_array.rs and column/writer/mod.rs::compare_greater - into a single shared is_decimal_descr() helper used by both call sites. Adds a regression test constructing a Decimal ByteArray column with positive/negative values whose two's-complement encoding exceeds a 1-byte statistics_truncate_length, verifying statistics are left untruncated and exact rather than corrupted by unsigned increment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
we have an existing PR for this |
Author
|
Thanks for the pointer — #11087 fixes the same min/max comparison bug, but doesn't touch |
Contributor
|
fyi @yuefdev |
Contributor
|
run benchmark arrow_writer env:
BENCH_FILTER: string |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/11073-arrow-decimal-byte-array-stats (6626f93) to 911721c (merge-base) diff Run configurationrun benchmark arrow_writer
env:
BENCH_FILTER: "string"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writer File an issue against this benchmark runner |
etseidl
reviewed
Sep 16, 2026
etseidl
left a comment
etseidl
left a comment
Contributor
There was a problem hiding this comment.
Thanks @DeviousCardi, I think this fix seems reasonable (good catch on the truncation). Given that min/max is on the hot path, I'd like to benchmark this some. My only nit right now is the comments are far too prolix.
Comment on lines
+436
to
+443
| /// 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// 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. |
I think the field name is pretty self explanatory.
Comment on lines
+827
to
+833
| /// | ||
| /// `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`). |
Contributor
There was a problem hiding this comment.
Suggested change
| /// | |
| /// `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`). |
Again, I think this level of explanation is unnecessary
Comment on lines
+6571
to
+6582
| /// 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// 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. | |
| /// 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). |
Comment on lines
+1360
to
+1364
| // 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| // 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. | |
| // As with FIXED_LEN_BYTE_ARRAY, do not truncate Decimal values |
| /// 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 { |
Comment on lines
+2003
to
+2011
| /// 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// 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. | |
| /// Returns `true` if the column described by `basic_type_info` is a Decimal column |
Comment on lines
+4589
to
+4594
| // 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| // 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. | |
| // See https://github.com/apache/arrow-rs/issues/11073 |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix/11073-arrow-decimal-byte-array-stats (6626f93) to 911721c (merge-base) diff Run configurationrun benchmark arrow_writer
env:
BENCH_FILTER: "string"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Author
|
Trimmed the verbose comments per your suggestions, thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #11073.
Writing a Decimal-logical-type column backed by an Arrow
BinaryArray/ByteArraythroughArrowColumnWriterproduced wrong min/max column statistics, because the statistics computation used plain unsigned lexicographicOrdcomparison on the raw two's-complement-encoded bytes. The directparquet::file::writercolumn-writer path (and theFixedSizeBinaryArraypath) already handled this correctly via a signed, decimal-aware comparator.Changes
parquet/src/column/writer/mod.rs: widenedcompare_greater_byte_array_decimalstopub(crate); added a sharedis_decimal_descr()helper (used by both this file'scompare_greaterand the Arrow writer) to avoid duplicating the decimal-type-detection check.parquet/src/arrow/arrow_writer/byte_array.rs:ByteArrayEncodernow detects decimal columns at construction and uses the signed comparator for both the running min/max computation and the cross-batch min/max merge.truncate_statistics/can_truncate_value(column/writer/mod.rs):BYTE_ARRAYdecimal columns are now excluded from statistics truncation, matching the existing guard already in place forFIXED_LEN_BYTE_ARRAYdecimals — truncating a decimal's two's-complement bytes with unsignedincrement()could otherwise reintroduce a corrupted bound for long decimals.Test plan
cargo test -p parquet --lib arrow_writer— 166 passedcargo test -p parquet --lib column::writer— 112 passedcargo clippy -p parquet --lib --all-targets— cleancargo build -p parquet— clean🤖 Generated with Claude Code