laskoviymishka commented on code in PR #1355:
URL: https://github.com/apache/iceberg-go/pull/1355#discussion_r3560961434
##########
manifest.go:
##########
@@ -2006,7 +2061,7 @@ func (d *dataFile) convertAvroValueToIcebergType(v any,
fieldID int) any {
return TimestampNano(v.(int64))
case atype.Decimal:
if r, ok := v.(*big.Rat); ok {
- scale := d.fieldIDToFixedSize[fieldID]
+ scale := d.fieldIDToDecimalScale[fieldID]
Review Comment:
Now that the read path reads `d.fieldIDToDecimalScale` here,
`d.fieldIDToFixedSize` has no reader left anywhere — the write path encodes off
`ManifestWriter.partFieldIDToSize` (schema-derived), not the per-`dataFile`
field. That makes the `fieldIDToFixedSize` argument to `NewDataFileBuilder`
silently a no-op: a caller passing 0 or a wrong width gets neither an error nor
a different result.
I'd drop `fieldIDToFixedSize` from the struct and the `NewDataFileBuilder`
signature; if it has to stay for the `hasFieldToIDMap` interface, at least
comment that it's unread on the write path so we're not handing callers a knob
that does nothing.
##########
table/internal/utils.go:
##########
@@ -290,7 +291,7 @@ func (d *DataFileStatistics) ToDataFile(opts DataFileOpts)
iceberg.DataFile {
fieldIDToLogicalType[field.FieldID] =
atype.TimestampMicros
case iceberg.DecimalType:
fieldIDToLogicalType[field.FieldID] =
atype.Decimal
- fieldIDToFixedSize[field.FieldID] =
rt.Scale()
+ fieldIDToFixedSize[field.FieldID] =
iceinternal.DecimalRequiredBytes(rt.Precision())
Review Comment:
This is the same class of bug the manifest fix addresses, but on the path
Arrow writers actually use to build `DataFile`s for commit — and it has no
test. A regression here would corrupt every decimal partition value written by
an Arrow writer without a single test failing.
I'd add one that runs a `DataFileStatistics` with a decimal partition column
through `ToDataFile` → `WriteManifest` → read back and asserts the decoded
value round-trips.
##########
manifest.go:
##########
@@ -1870,13 +1896,41 @@ func convertUUIDValue(v any) any {
return v
}
-func padOrTruncateBytes(bytes []byte, size int) []byte {
- if len(bytes) >= size {
- return bytes[len(bytes)-size:]
+func fitDecimalBytes(bytes []byte, size int) ([]byte, error) {
+ if len(bytes) == 0 {
Review Comment:
The `len(bytes) == 0` branch is unreachable — `DecimalLiteral.MarshalBinary`
always emits at least one byte (zero gives `[0x00]`), so no caller can deliver
an empty slice. Meanwhile there's no `size <= 0` guard: called with `size == 0`
and non-empty bytes, it falls through to the trim branch and `retained[0]`
panics on the empty slice.
`encodeDecimalBytes` catches `fixedSize <= 0` today so it's not reachable in
production, but since this is unexported and test-callable I'd swap the dead
empty-bytes check for a `size <= 0` guard that returns an error.
##########
manifest_test.go:
##########
@@ -2044,6 +2047,202 @@ func
TestReadManifestDecodesNilLogicalPartitionValueFromNullableUnion(t *testing
}
}
+func TestFitDecimalBytesAllowsOnlyRedundantSignExtension(t *testing.T) {
+ tests := []struct {
+ name string
+ bytes []byte
+ size int
+ want []byte
+ wantErr bool
+ }{
+ {
+ name: "trims redundant positive sign extension",
+ bytes: []byte{0x00, 0x00, 0x01},
+ size: 2,
+ want: []byte{0x00, 0x01},
+ },
+ {
+ name: "trims redundant negative sign extension",
+ bytes: []byte{0xff, 0xff, 0x80},
+ size: 2,
+ want: []byte{0xff, 0x80},
+ },
+ {
+ name: "rejects positive sign change",
+ bytes: []byte{0x00, 0x80},
+ size: 1,
+ wantErr: true,
+ },
+ {
+ name: "rejects negative sign change",
+ bytes: []byte{0xff, 0x7f},
+ size: 1,
+ wantErr: true,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got, err := fitDecimalBytes(tt.bytes, tt.size)
+ if tt.wantErr {
+ if err == nil {
+ t.Fatal("expected error")
+ }
+
+ return
+ }
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Equal(got, tt.want) {
+ t.Fatalf("fitDecimalBytes() = %v, want %v",
got, tt.want)
+ }
+ })
+ }
+}
+
+func TestAvroEncodePartitionDataUsesDeclaredDecimalFixedSize(t *testing.T) {
+ decimalFieldID := 1000
+ maxPrecision38 := new(big.Int).Sub(new(big.Int).Exp(big.NewInt(10),
big.NewInt(38), nil), big.NewInt(1))
+
+ tests := []struct {
+ name string
+ value any
+ precision int
+ want []byte
+ wantErr bool
+ }{
+ {
+ name: "pads positive decimal",
+ value: Decimal{Val: decimal128.FromI64(1), Scale:
2},
+ precision: 10,
+ want: []byte{0x00, 0x00, 0x00, 0x00, 0x01},
+ },
+ {
+ name: "pads zero decimal",
+ value: Decimal{Val: decimal128.FromI64(0), Scale:
2},
+ precision: 10,
+ want: []byte{0x00, 0x00, 0x00, 0x00, 0x00},
+ },
+ {
+ name: "sign extends negative decimal",
+ value: Decimal{Val: decimal128.FromI64(-1), Scale:
2},
+ precision: 10,
+ want: []byte{0xff, 0xff, 0xff, 0xff, 0xff},
+ },
+ {
+ name: "keeps max precision decimal",
+ value: DecimalLiteral{Val:
decimal128.FromBigInt(maxPrecision38), Scale: 0},
+ precision: 38,
+ want: []byte{
+ 0x4b, 0x3b, 0x4c, 0xa8, 0x5a, 0x86, 0xc4, 0x7a,
Review Comment:
Could we drop a one-liner on what these bytes are — e.g. `//
two's-complement big-endian of 10^38 - 1`? A bare 16-byte literal is the kind
of thing that's impossible to verify or update later without recomputing it by
hand.
##########
manifest.go:
##########
@@ -1848,18 +1864,28 @@ func convertTimestampMicrosValue(v any) any {
return v
}
-func convertDecimalValue(v any) any {
- if dec, ok := v.(Decimal); ok {
- fixedSize := internal.DecimalRequiredBytes(len(dec.String()))
- bytes, err := DecimalLiteral(dec).MarshalBinary()
- if err != nil {
- return v
- }
+func convertDecimalValue(v any, fixedSize int) (any, error) {
Review Comment:
`convertDecimalValue` now handles both `Decimal` (from `NewDataFileBuilder`)
and `DecimalLiteral` (from a round-trip through `initPartitionData`) — that
dual case is a bit of a symptom of the partition value model not converging on
one type. Not blocking, but a one-line comment on when each shows up would save
the next reader the archaeology. wdyt?
##########
manifest_test.go:
##########
@@ -2044,6 +2047,202 @@ func
TestReadManifestDecodesNilLogicalPartitionValueFromNullableUnion(t *testing
}
}
+func TestFitDecimalBytesAllowsOnlyRedundantSignExtension(t *testing.T) {
+ tests := []struct {
+ name string
+ bytes []byte
+ size int
+ want []byte
+ wantErr bool
+ }{
+ {
+ name: "trims redundant positive sign extension",
+ bytes: []byte{0x00, 0x00, 0x01},
+ size: 2,
+ want: []byte{0x00, 0x01},
+ },
+ {
+ name: "trims redundant negative sign extension",
+ bytes: []byte{0xff, 0xff, 0x80},
+ size: 2,
+ want: []byte{0xff, 0x80},
+ },
+ {
+ name: "rejects positive sign change",
+ bytes: []byte{0x00, 0x80},
+ size: 1,
+ wantErr: true,
+ },
+ {
+ name: "rejects negative sign change",
+ bytes: []byte{0xff, 0x7f},
+ size: 1,
+ wantErr: true,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got, err := fitDecimalBytes(tt.bytes, tt.size)
+ if tt.wantErr {
+ if err == nil {
+ t.Fatal("expected error")
+ }
+
+ return
+ }
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Equal(got, tt.want) {
+ t.Fatalf("fitDecimalBytes() = %v, want %v",
got, tt.want)
+ }
+ })
+ }
+}
+
+func TestAvroEncodePartitionDataUsesDeclaredDecimalFixedSize(t *testing.T) {
Review Comment:
The vectors here are solid, but they only hit precision 10 and 38 — I'd add
the `requiredLength` tier boundaries too, since those are exactly where an
off-by-one in `DecimalRequiredBytes` would show up: precision 1 (size 1, value
5 → `[0x05]`), precision 9 (size 4), and a negative at precision 18 (size 9,
sign-extended).
While we're here, `TestFitDecimalBytesAllowsOnlyRedundantSignExtension` only
exercises the trim path — a direct padding case like `{0x01}` size 3 →
`{0x00,0x00,0x01}` plus a negative `{0xff}` → `{0xff,0xff,0xff}` would cover
the pad branch directly rather than only through the encode test.
##########
manifest.go:
##########
@@ -1916,6 +1970,7 @@ type dataFile struct {
fieldIDToLogicalType map[int]string
fieldIDToPartitionData map[int]any
fieldIDToFixedSize map[int]int
+ fieldIDToDecimalScale map[int]int
Review Comment:
`fieldIDToDecimalScale` only ever gets populated by the `ReadEntry` setter
path — `NewDataFileBuilder` doesn't take a scale map. So a `dataFile` built via
the constructor and read back with `Partition()` without going through
`ManifestReader` reads `scale=0`, and `(123, scale=2)` silently decodes as
`123`.
Mirror image of the dead `fieldIDToFixedSize` above — I'd either add a
`fieldIDToDecimalScale` param to `NewDataFileBuilder` or derive scale from the
spec+schema in the constructor. wdyt?
##########
manifest.go:
##########
@@ -376,7 +376,7 @@ func (m *manifestFile) FetchEntries(fs iceio.IO,
discardDeleted bool) (_ []Manif
return ReadManifest(m, f, discardDeleted)
}
-func getFieldIDMap(sc *avro.Schema) (map[string]int, map[int]string,
map[int]int) {
+func getFieldIDMap(sc *avro.Schema) (map[string]int, map[int]string,
map[int]int, map[int]int) {
Review Comment:
`getFieldIDMap` now returns four positional maps, two of them `map[int]int`
— at the call sites the size map and the scale map are indistinguishable by
type, and one site already discards the fourth with `_`. I'd return the
existing `dataFileFieldMaps` struct (or named returns) so a future caller can't
swap size and scale by accident. wdyt?
##########
manifest.go:
##########
@@ -1870,13 +1896,41 @@ func convertUUIDValue(v any) any {
return v
}
-func padOrTruncateBytes(bytes []byte, size int) []byte {
- if len(bytes) >= size {
- return bytes[len(bytes)-size:]
+func fitDecimalBytes(bytes []byte, size int) ([]byte, error) {
+ if len(bytes) == 0 {
+ return make([]byte, size), nil
+ }
+
+ if len(bytes) == size {
+ return bytes, nil
+ }
+
+ if len(bytes) < size {
+ signByte := byte(0x00)
+ if bytes[0]&0x80 != 0 {
+ signByte = 0xff
+ }
+ padded := make([]byte, size)
+ for i := range padded[:size-len(bytes)] {
+ padded[i] = signByte
+ }
+ copy(padded[size-len(bytes):], bytes)
+
+ return padded, nil
}
- padded := slices.Grow(bytes, size-len(bytes))
- return append(make([]byte, size-len(bytes)), padded...)
+ retained := bytes[len(bytes)-size:]
+ signByte := byte(0x00)
+ if retained[0]&0x80 != 0 {
+ signByte = 0xff
+ }
+ for _, b := range bytes[:len(bytes)-size] {
+ if b != signByte {
+ return nil, fmt.Errorf("decimal value does not fit
fixed size %d", size)
Review Comment:
Small thing: this reports only `size`, so at the top level you get `decimal
value does not fit fixed size 5` with no hint whether the value was 6 bytes or
16. Including `len(bytes)` — `decimal value of %d bytes does not fit fixed size
%d` — makes it debuggable at a glance.
--
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]
