ajiteshsingh opened a new pull request, #2009:
URL: https://github.com/apache/iceberg-go/pull/2009
Closes #1876.
## Summary
- Byte-reverse INT32/INT64-backed decimal row-group statistics before
storing them as Iceberg bounds.
- Leave `FIXED_LEN_BYTE_ARRAY`-backed decimals untouched — Parquet already
stores those big-endian.
- Add row-group pruning coverage for all three physical types, plus a test
that reads statistics a real Parquet writer emitted rather than hand-written
bytes.
## Why
`TestRowGroup` stored `stats.EncodeMin()`/`EncodeMax()` straight into the
bounds maps, which are later decoded with `iceberg.LiteralFromBytes`. Parquet
plain-encodes INT32/INT64 **little-endian**; an Iceberg bound is **big-endian**
two's complement. So an unscaled `659` came back as `-1828585472`:
```
parquet stats bytes 93 02 00 00
decoded as an Iceberg bound 0x93020000 = -1828585472
```
Every row group then reports a hugely negative range, `VisitEqual` and
`VisitGreater*` return `rowsCannotMatch` for all of them, and the scan silently
yields zero rows.
Byte order is the *only* difference between the two encodings, so reversing
the stat bytes is a complete conversion rather than a workaround:
```go
lower, upper := stats.EncodeMin(), stats.EncodeMax()
if intBackedDecimal(stats.Descr()) {
slices.Reverse(lower)
slices.Reverse(upper)
}
```
This takes the issue's step 2 directly instead of landing step 1 first.
Omitting the bounds is also correct — a missing bound degrades to
`rowsMightMatch` — but it disables decimal row-group pruning entirely, and
decimals are exactly the columns people filter on. Three details make the
one-liner sufficient:
- `DecimalLiteral.UnmarshalBinary` keys off the sign bit of `data[0]` and
then does full-width two's complement, so Iceberg's minimum-length rule does
not apply and the fixed 4- or 8-byte width decodes as-is.
- `EncodeMin`/`EncodeMax` return a freshly allocated buffer (`plainEncode`
copies), so reversing in place cannot disturb the row group metadata.
- arrow-go plain-encodes little-endian on big-endian hosts too
(`encoding_utils_big_endian.go` uses an explicit `binary.LittleEndian`), so the
conversion is not host-architecture dependent — relevant given the s390x build
job.
## Tests
`TestInclusiveMetricsEvalIntBackedDecimalRowGroup` covers INT32, INT64, and
an FLBA control over a row group spanning `-6.59` to `123.45` at scale 2 — the
negative minimum exercises the sign-bit decode path. Each case asserts in both
directions, so neither a corrupted bound nor a silently dropped one passes:
| Assertions | Catch |
|---|---|
| `== min`, `== max` keep the row group | a bound decoded wrongly, or
swapped with its partner |
| `< min`, `> max` prune | a bound dropped rather than converted |
| `== min-1`, `== max+1` prune | `VisitEqual`'s own pruning branches |
`TestInclusiveMetricsEvalRealParquetDecimalRowGroup` writes a real Parquet
file with an INT32-backed `decimal(9,2)` column and prunes against the metadata
the writer produced, so the hand-written fixture cannot drift from what Parquet
actually emits. It guards its own relevance with
`require.True(intBackedDecimal(stats.Descr()))`, failing loudly rather than
silently stopping covering the bug if a future arrow-go changes physical-type
selection.
Each assertion was verified to be load-bearing by mutating the source:
removing the conversion, dropping the bounds instead, swapping lower/upper,
applying the reversal to FLBA as well, and disabling each `VisitEqual` bound
check. All are caught.
## Checks
- `go test ./table/ -count=1`
- `make test-race` — 34 packages, 0 data races
- `make test-assert`
- `golangci-lint run --timeout=10m` (v2.12.2)
- `gofmt` under Go 1.26.1
- `go build ./...`, `go vet ./...`
The one failure in the full race run, `puffin`'s
`TestReaderPreservesLZ4ChecksumError`, reproduces on unmodified `main` and is
unrelated to this change.
## Note for reviewers
The bound is decoded with the **Iceberg field's** scale, not the Parquet
logical type's. Iceberg forbids changing a decimal's scale during schema
evolution (`types.go:1182` enforces equal scale and allows only precision
widening), so these agree for tables written through Iceberg. The issue does
mention a Snowflake table whose Parquet logical *precision* (3) disagrees with
the Iceberg field (38); precision does not affect decoding, and scale matched
there. If reviewers would prefer the bound rescaled from the Parquet logical
scale defensively, I'm happy to add that.
AI Disclosure: Assisted by Claude (Opus 5).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
