laskoviymishka commented on code in PR #1312:
URL: https://github.com/apache/iceberg-go/pull/1312#discussion_r3559026700
##########
types.go:
##########
@@ -200,16 +205,10 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- prec, err := strconv.Atoi(matches[1])
- if err != nil {
- return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
- }
- scale, err := strconv.Atoi(matches[2])
- if err != nil {
- return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
- }
- if err := validateDecimalPrecisionScale(prec,
scale); err != nil {
- return fmt.Errorf("%w: %w",
ErrInvalidTypeString, err)
+ prec, _ := strconv.Atoi(matches[1])
Review Comment:
Same one zeroshade flagged — agreed it needs fixing. Dropping the `Atoi`
errors here is only safe by accident: on `decimal(99999999999999999999, 0)` the
parse saturates to `math.MaxInt`, and the `prec > 38` bound happens to catch
it, so the message comes out as "precision must be between 1 and 38:
9223372036854775807" instead of "value out of range".
The tell is that the `fixed` path two blocks up (line 196) *does* propagate
the `Atoi` error — this path used to as well. If `maxDecimalPrecision` ever
moves, the accidental safety disappears and it becomes a real bypass.
I'd restore both err checks to match the fixed path, and add a
`decimal(99999999999999999999, 0)` case asserting "value out of range" — the
fixed overflow case is already tested, decimal isn't.
##########
types.go:
##########
@@ -200,16 +205,10 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- prec, err := strconv.Atoi(matches[1])
- if err != nil {
- return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
- }
- scale, err := strconv.Atoi(matches[2])
- if err != nil {
- return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
- }
- if err := validateDecimalPrecisionScale(prec,
scale); err != nil {
- return fmt.Errorf("%w: %w",
ErrInvalidTypeString, err)
+ prec, _ := strconv.Atoi(matches[1])
+ scale, _ := strconv.Atoi(matches[2])
+ if err := validateDecimalParams(prec, scale);
err != nil {
Review Comment:
Same class as the `fixed[0]` note above, decimal side.
`validateDecimalParams` on the parse path now rejects `decimal(0, 0)` and any
`scale > precision`, but Java's `DecimalType.of` only checks `precision <= 38`
— it happily constructs `decimal(2, 5)`. So a Java-written table with `scale >
precision` stops unmarshalling here.
If we go permissive-on-read, this and the fixed case move together: strict
in the constructors, lenient in `UnmarshalJSON`.
##########
literals.go:
##########
@@ -1180,10 +1180,13 @@ func (g GeoLiteral) MarshalBinary() ([]byte, error) {
type FixedLiteral []byte
func (FixedLiteral) Comparator() Comparator[[]byte] { return bytes.Compare }
-func (f FixedLiteral) Type() Type { return FixedType{len:
len(f)} }
-func (f FixedLiteral) Value() []byte { return []byte(f) }
-func (f FixedLiteral) Any() any { return f.Value() }
-func (f FixedLiteral) String() string { return string(f) }
+
+// Empty fixed literals can arise from zero values or empty unmarshaled data.
+// Report fixed[1] as a placeholder so Type() stays non-panicking.
+func (f FixedLiteral) Type() Type { return FixedTypeOf(max(1, len(f))) }
Review Comment:
Trading the panic for a placeholder makes sense, but `fixed[1]` is a claim
about the data that isn't true — a zero-length `FixedLiteral` reports length 1
while the underlying bytes are empty. Anything using `lit.Type()` for a length
decision (Parquet schema, equality, scan planning) gets the wrong answer, and
`lit.To(FixedType{len: 1})` fails since `0 != 1`.
Since `Type()` is meant to report what the literal *is*, not construct a
type for registration, I'd return `FixedType{len: len(f)}` via the struct
literal directly and skip the `FixedTypeOf` floor. If the floor is deliberate
to dodge the constructor panic, the comment should say the length is a
non-representative placeholder.
##########
types.go:
##########
@@ -571,17 +570,17 @@ func (m *MapType) UnmarshalJSON(b []byte) error {
return nil
}
-func validateFixedLength(length int) error {
- if length < 0 {
- return fmt.Errorf("fixed length must be non-negative, got %d",
length)
+func validateFixedLength(n int) error {
+ if n <= 0 {
Review Comment:
This now runs on the JSON parse path, and that's where I get nervous. The
spec says `fixed(L)` with no stated lower bound on L, and Java's
`Types.FixedType.ofLength` does no validation at all — so a metadata JSON
containing `fixed[0]`, written by Java or older iceberg-go, used to unmarshal
fine and now hard-errors.
Enforcing `>= 1` inside `FixedTypeOf` is fine — that's our constructor, our
contract. But rejecting it when *reading* someone else's table is a
compatibility decision. I'd keep the parse path permissive (`n < 0`) and
enforce the stricter bound only in the constructor. If the strict read is
intentional, could we say so in the PR description? wdyt?
##########
table/substrait/substrait.go:
##########
@@ -263,9 +263,9 @@ func toByteSliceSubstraitLiteral[T []byte | types.UUID](v
T) expr.Literal {
// toDecimalLiteral converts an iceberg.DecimalLiteral into a Substrait decimal
// literal. Precision is taken from the bound field's iceberg.DecimalType
(typ),
// NOT from v.Type(). DecimalLiteral does not carry the originating column's
-// precision, so v.Type() always reports a hardcoded precision of 9 (see
+// precision, so v.Type() reports a placeholder precision (see
// https://github.com/apache/iceberg-go/issues/1028). If typ is not a
-// DecimalType, this falls back to v.Type()'s (hardcoded) precision.
+// DecimalType, this falls back to v.Type()'s placeholder precision.
Review Comment:
This comment is now factually off — the fallback at line 270 is `precision
:= int32(9)`, which never calls `v.Type()`. The old "hardcoded precision of 9"
wording was accurate. I'd make it something like "falls back to a hardcoded
placeholder precision of 9 (independent of `v.Type()`)".
##########
types_test.go:
##########
@@ -73,14 +73,74 @@ func TestTypesBasic(t *testing.T) {
}
}
+func TestTypeUnmarshalRejectsInvalidFixedAndDecimal(t *testing.T) {
Review Comment:
Small cleanups while we're here: this table overlaps
`TestDecimalTypeInvalidParse` (both cover zero precision, precision too large,
negative scale) — `scale > precision` is the only genuinely new coverage, so
I'd fold it into the existing test rather than duplicate. Also, unlike the
existing test, this one has no `name` field, so `t.Run` uses the raw type
string ("decimal(2, 5)"), whose parens/commas need escaping for `go test -run`.
A `name` field would fix that.
##########
literals.go:
##########
@@ -1307,13 +1310,18 @@ func (DecimalLiteral) Comparator() Comparator[Decimal] {
}
}
-// Type returns a DecimalType built from the literal's scale and a hardcoded
-// precision of 9. The precision is NOT the originating column's declared
-// precision; DecimalLiteral does not carry precision. Callers that need the
-// real column precision must consult the bound field's type rather than
-// lit.Type(). See https://github.com/apache/iceberg-go/issues/1028.
+// Type returns a DecimalType built from the literal's scale and a placeholder
+// precision that is at least 9. The precision is NOT the originating column's
+// declared precision; DecimalLiteral does not carry precision. Callers that
+// need the real column precision must consult the bound field's type rather
+// than lit.Type(). See https://github.com/apache/iceberg-go/issues/1028.
func (d DecimalLiteral) Type() Type {
- return DecimalType{precision: 9, scale: d.Scale}
+ // Clamp the placeholder scale into Iceberg's valid schema range so odd
+ // literal values still report a non-panicking placeholder type.
+ scale := min(maxDecimalPrecision, max(0, d.Scale))
Review Comment:
Same shape as the fixed case. Clamping `scale` to dodge the `DecimalTypeOf`
panic means a literal with `Scale: 39` reports `decimal(38, 38)` — a caller
doing `lit.Type().(DecimalType).Scale()` expecting 39 silently gets 38. The old
code was wrong about precision but at least honest about scale.
Not a blocker, but I'd either return the struct literal directly
(`DecimalType{precision: max(9, d.Scale), scale: d.Scale}`) or document that
`Type()` is a clamped, non-representative placeholder. wdyt?
--
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]
