laskoviymishka commented on code in PR #1585:
URL: https://github.com/apache/iceberg-go/pull/1585#discussion_r3681111655
##########
literals.go:
##########
@@ -193,13 +193,16 @@ func LiteralFromBytes(typ Type, data []byte) (Literal,
error) {
return GeoLiteral{val: data, typ: typ}, nil
case FixedType:
- if len(data) != t.Len() {
+ if len(data) < t.Len() {
// looks like some writers will write a prefix of the
fixed length
// for lower/upper bounds instead of the full length.
so let's pad
// it out to the full length if unpacking a fixed
length literal
padded := make([]byte, t.Len())
copy(padded, data)
data = padded
+ } else if len(data) > t.Len() {
Review Comment:
the reject itself is right, but I think there's a subtle blast radius here
worth settling before merge.
I traced it through: every `LiteralFromBytes` call in `table/evaluators.go`
wraps the error in `panic()`, and `VisitExpr` recovers that into a
scan-aborting error. so an oversized fixed bound on one manifest entry now
takes down the entire scan — where before it truncated and kept going. Java and
PyIceberg both tolerate any-length fixed bounds and just keep evaluating, so
this ends up being a Go-only read regression.
I'd keep the strict error here but degrade at the callers: when
`LiteralFromBytes` returns `ErrInvalidBinSerialization` for a bound, have the
evaluator treat that bound as unknown and return `rowsMightMatch` instead of
panicking. wdyt?
##########
literals_test.go:
##########
@@ -1201,6 +1202,41 @@ func TestUnmarshalBinary(t *testing.T) {
}
}
+func TestLiteralFromBytesFixedWidth(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ data []byte
+ want iceberg.FixedLiteral
+ wantErr bool
+ }{
+ {name: "exact", data: []byte{1, 2, 3, 4}, want:
iceberg.FixedLiteral{1, 2, 3, 4}},
+ {name: "short", data: []byte{1, 2, 3}, want:
iceberg.FixedLiteral{1, 2, 3, 0}},
+ {name: "empty", data: nil, wantErr: true},
Review Comment:
small thing on the table: the `empty` case passes because `data: nil` trips
the nil guard at the top of `LiteralFromBytes`, before the `FixedType` branch
ever runs — so it's not actually exercising the new path, and the name is a bit
misleading.
the interesting gap is a non-nil `[]byte{}`, which hits the pad branch and
comes back as `FixedLiteral{0,0,0,0}` today with nothing covering it. I'd
rename this one to `nil` and add a zero-length case documenting that pad
behavior.
##########
literals_test.go:
##########
@@ -1201,6 +1202,41 @@ func TestUnmarshalBinary(t *testing.T) {
}
}
+func TestLiteralFromBytesFixedWidth(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ data []byte
+ want iceberg.FixedLiteral
+ wantErr bool
+ }{
+ {name: "exact", data: []byte{1, 2, 3, 4}, want:
iceberg.FixedLiteral{1, 2, 3, 4}},
+ {name: "short", data: []byte{1, 2, 3}, want:
iceberg.FixedLiteral{1, 2, 3, 0}},
+ {name: "empty", data: nil, wantErr: true},
+ {name: "one byte oversized", data: []byte{1, 2, 3, 4, 5},
wantErr: true},
+ {name: "large oversized", data: make([]byte, 100), wantErr:
true},
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ literal, err :=
iceberg.LiteralFromBytes(iceberg.FixedTypeOf(4), test.data)
+ if test.wantErr {
+ require.ErrorIs(t, err,
iceberg.ErrInvalidBinSerialization)
+ if len(test.data) > 4 {
Review Comment:
the `> 4` guard hard-codes the type width — if someone bumps this to
`FixedTypeOf(8)` the message assertions silently stop running and the oversized
cases go green without checking anything.
I'd move the expected substring onto the table (an `errContains` field on
the oversized rows) so each case asserts its own message and the conditional
drops out.
--
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]
