Repository navigation
fix(arrow/array): validate bufferless run-end encoded arrays - #1421
Merged
Merged
Conversation
REE arrays have no physical parent buffers, but their layout declares an always-nil placeholder. Builders and C data imports omit that placeholder, so generic validation rejected valid arrays before reaching the REE checks. Accept zero-buffer REE data, including extension storage, while retaining the existing single-nil representation used by constructors and IPC readers. Preserve checks for excess or non-nil parent buffers, metadata, children, and run ends. Leave NullType validation unchanged. Document the optional placeholder and cover builders, constructors, both concatenation orders, empty and sliced arrays, extension storage, invalid buffers, and duplicate run ends.
zeroshade
added a commit
that referenced
this pull request
Oct 26, 2026
…ons (#1409) ### Rationale for this change Found while fixing the 32-bit test failures in #1408. Decimal conversions are wrong on 32-bit platforms. `decimal128.Num` and `decimal256.Num` use 64-bit limbs, but their `big.Int` conversions assumed `big.Word` is also 64 bits. On 386 it is 32 bits, so: - `decimal128.FromBigInt` read `b[0]`/`b[1]` as the low and high limbs (`decimal128.go:95–97`), misplacing words and dropping the rest; - `decimal128`'s `toBigIntPositive` built `[]big.Word{big.Word(n.lo), big.Word(n.hi)}` (`decimal128.go:362`), truncating each limb; - `decimal256.FromBigInt` copied word *i* into limb *i* (`decimal256.go:370`), misplacing words and panicking when a value needed more than four native words; - `decimal256`'s `toBigIntPositive` truncated its limbs the same way (`decimal256.go:379`). Anything going through `big.Int` (string formatting and parsing, rescaling, `GetOneForMarshal`, CSV decimal parsing) produced wrong values or spurious precision errors on 386, e.g. `TestDecimal128Slice` printed `-1717986918` instead of `-7.378697629e+18`, and `TestDecimal128GetOneForMarshal` turned `1234567890.123456789` into `437278697.08398250845211745659132109676`. The run-end-encoded validation fix that was originally part of this PR has moved to #1421. ### What changes are included in this PR? - **`arrow/decimal128`, `arrow/decimal256`:** when `bits.UintSize == 32`, combine pairs of 32-bit `big.Word`s into each 64-bit limb in `FromBigInt`, and split each limb into two words in `toBigIntPositive`. `bits.UintSize` is a constant, so 64-bit builds compile the existing code unchanged. Also corrects the `decimal128.FromBigInt` doc (it panics above 127 bits, as the code already does). - **CI:** the 386 job also runs the decimal packages' tests. ### Are these changes tested? Yes. - **On `GOARCH=386`:** `TestDecimal128Slice`, `TestDecimal128StringRoundTrip`, `TestDecimal128GetOneForMarshal`, `TestDecimal256Slice`, `TestDecimal256GetOneForMarshal`, all nine `TestDecimal/TestWithNulls#01` precisions, `TestDecimalScalarStringCastUsesExactScaling` and CSV's `TestReadCSVDecimalCols` all failed before and pass now. The `TestDecimalToBigInt` tests in both packages gain word-boundary cases with independently computed expectations, which fail on 386 without the fix (decimal256 panics). `arrow/decimal128`, `arrow/decimal256` and `arrow/decimal/...` pass in full on 386. - **amd64:** `arrow/decimal128`, `arrow/decimal256`, `arrow/decimal/...` pass. - `go vet` and `gofmt -l` are clean on the changed packages. ### Are there any user-facing changes? Yes: decimal values convert correctly on 32-bit platforms. No API changes.
zeroshade
added a commit
that referenced
this pull request
Oct 26, 2026
### Rationale for this change The macOS ARM64 Test jobs are running into their `timeout-minutes`. In the last 40 Test runs, successful `ARM64 macOS 14 Go 1.26` jobs took 14–17 minutes against a 20-minute limit, and two jobs were cancelled at the limit today (#1414's `ARM64 macOS 14 Go 1.26`, #1421's `ARM64 macOS 14 Go 1.26 - CGO`), both during the last command of `ci/scripts/test.sh`: `go test -race -tags assert,noasm …/parquet/file`. All tests that had run passed. On macOS, `parquet/file` runs in its own invocation, once per tag set (`assert`, then `assert,noasm`); under `-race` that takes about 90 seconds or more each time. But `parquet/file` has no `noasm`-specific code: every `noasm` variant lives in `parquet/internal/bmi`, `parquet/internal/encoding`, `parquet/internal/utils` and `parquet/metadata`, which are still tested in both passes. The second run repeats the same tests. ### What changes are included in this PR? `ci/scripts/test.sh` runs the macOS-only `parquet/file` invocation once, with `-tags assert`, after both passes over the other parquet packages. Other platforms are unchanged (`parquet/file` stays part of `./...` in both passes there). ### Are these changes tested? `bash -n` passes. A dry run of the script with `go` stubbed to report darwin and print each `go test` command shows the arrow passes unchanged, the other parquet packages in both `assert` and `assert,noasm` passes, and `parquet/file` once with `assert`. This PR's own macOS jobs exercise it. ### Are there any user-facing changes? No.
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
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.
Rationale for this change
Split out of #1409.
ValidateFullrejects run-end-encoded arrays from the builder. A REE array fromNewRunEndEncodedBuilder(or aConcatenateof such arrays) fails withexpected at least 1 buffers for run_end_encoded<...>, got 0, on every platform. The REE layout declares one always-null placeholder buffer (datatype_encoded.go:40);NewRunEndEncodedArrayand the IPC reader supply anilplaceholder, but the builder and C Data import produce zero buffers. The Arrow spec says REE arrays have no physical buffers, so both representations are legitimate.What changes are included in this PR?
arrow/array/validate.goaccepts a REE array (including extension types with REE storage) that has no buffers. Arrays with anilplaceholder go through the existing checks as before, and non-nil or extra parent buffers are still rejected. Constructors, layouts, IPC and C Data are unchanged.Are these changes tested?
Yes. New
TestRunEndEncodedValidateConstructedArrayschecks that builder-produced, empty, sliced, constructor-built,Concatenated and REE-extension arrays passValidateFull. The builder,Concatenate, empty, sliced and extension cases fail before the fix; constructor cases passed already. Existing rejection cases (bad run ends, non-nil/extra parent buffers) still fail validation.go vetandgofmt -lare clean onarrow/array.Are there any user-facing changes?
Yes:
ValidateFullaccepts run-end-encoded arrays produced by the builder,Concatenateand C Data import. No API changes.