zeroshade opened a new pull request, #1409:
URL: https://github.com/apache/arrow-go/pull/1409
### Rationale for this change
Two independent bugs, found while fixing the 32-bit test failures in #1408.
**1. 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`.
**2. `ValidateFull` rejects run-end-encoded arrays from the builder.** A REE
array from `NewRunEndEncodedBuilder` (or a `Concatenate` of such arrays) fails
with `expected 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`); `NewRunEndEncodedArray` and the IPC reader supply a
`nil` placeholder, 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/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).
- **`arrow/array/validate.go`:** accept a REE array (including extension
types with REE storage) that has no buffers. Arrays with a `nil` placeholder 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.
- **Decimal, 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.
- **REE validation:** new `TestRunEndEncodedValidateConstructedArrays`
checks that builder-produced, empty, sliced, constructor-built, `Concatenate`d
and REE-extension arrays pass `ValidateFull`. 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.
- **amd64:** `arrow/array`, `arrow/decimal128`, `arrow/decimal256`,
`arrow/decimal/...`, `arrow/scalar`, `arrow/ipc` and `arrow/compute/...` pass;
`arrow/cdata` passes with cgo and `-tags test`.
- `go vet` and `gofmt -l` are clean on the changed packages.
Not addressed here; these still fail on 386, unrelated to these changes:
- `arrow/array`: `TestStructValidateRejectsOffsetLengthOverflow`,
`TestTimeJSON/time64[us]` and `TestTimeJSON/time64[ns]`;
- `arrow/scalar`: `TestTimeScalarsMakeScalar` and `TestToScalar`, which
expect Go `int`/`uint` to map to 64-bit types;
- `arrow/array`'s REE concat tests, fixed by #1408.
### Are there any user-facing changes?
Yes: decimal values convert correctly on 32-bit platforms, and
`ValidateFull` accepts run-end-encoded arrays produced by the builder,
`Concatenate` and C Data import. No API changes.
--
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]
