Conversation
ByteArrayEncoder computed min/max with plain byte order, so decimals written from a byte array to a BYTE_ARRAY DECIMAL column got unsigned statistics (-1, 0, 1 gave min 0 and max -1). Use the same signed comparison as the generic column writer for decimal columns. Closes apache#11073
yuefdev
force-pushed
the
fix-byte-array-decimal-stats
branch
from
September 14, 2026 23:38
c15a048 to
5c59f2a
Compare
Contributor
|
@neilconway do you have time to help look at this, since you worked on something similar recently |
4 tasks
Contributor
|
run benchmark arrow_writer env:
BENCH_FILTER: string |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-byte-array-decimal-stats (5c59f2a) to fbdbf93 (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 |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix-byte-array-decimal-stats (5c59f2a) to fbdbf93 (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 |
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.
Which issue does this PR close?
Rationale for this change
ByteArrayEncoder, used by the arrow writer forBinary,Utf8and the other byte array types, computes min/max using plain byte order. For aBYTE_ARRAYcolumn annotated asDECIMALthe values are big-endian two's complement, so the statistics come out as if they were unsigned: writing -1, 0, 1 gives min0x00and max0xFF.The non-arrow writer and the
FixedSizeBinarypath already compare decimals withcompare_greater_byte_array_decimals, which is why only this path was affected.What changes are included in this PR?
ByteArrayEncodernow notes whether the column is a decimal (logical or converted type) and, if so, usescompare_greater_byte_array_decimalswhen computing and merging min/max. Everything else keeps the plain byte comparison.compute_min_maxtakes the comparison as a generic parameter, so there's no extra per-value branch.compare_greater_byte_array_decimalsis nowpub(crate).Are these changes tested?
Yes.
test_byte_array_decimal_statisticswrites -1, 0, 1 from aBinaryArrayinto aBYTE_ARRAYDECIMAL(2, 0)column. It fails onmain(min is[0]instead of[255]) and passes with this change. The reproducer from the issue now reports(-1, 1)for all three paths.Ran locally on Windows with Rust 1.98.1:
cargo test -p parquet: all passingcargo test -p parquet --all-features --no-fail-fast: lib (1436) and all integration suites pass.parquet-fromcsv'stest_command_helpfails, but it fails onmaintoo here (CRLF checkout of the help text viacore.autocrlf). The all-features doc tests wouldn't build on this machine (stale artefacts intarget/and a pagefile limit), so I'm leaving those to CI.cargo clippy -p parquet --all-targets --all-features -- -D warningsandcargo fmt --all -- --check: cleanAre there any user-facing changes?
Statistics for
BYTE_ARRAYdecimal columns written through the arrow writer are now correct. No API changes.AI assistance was used for the fix, the test and this description.