zeroshade commented on code in PR #1312:
URL: https://github.com/apache/iceberg-go/pull/1312#discussion_r3483222966
##########
types.go:
##########
@@ -183,12 +185,16 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
default:
switch {
case strings.HasPrefix(typename, "fixed"):
- matches :=
regexFromBrackets.FindStringSubmatch(typename)
+ matches :=
fixedRegex.FindStringSubmatch(typename)
if len(matches) != 2 {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
n, _ := strconv.Atoi(matches[1])
Review Comment:
[Major] The `strconv.Atoi` error is dropped here, and `validateFixedLength`
only rejects `n <= 0`. On overflow `Atoi` returns `math.MaxInt` together with
an `ErrRange`, so `fixed[99999999999999999999]` is accepted as
`fixed[math.MaxInt]` instead of being rejected - a validation bypass in the
exact path this PR hardens. (The decimal branch below is incidentally safe
because its `> 38` upper bound catches the saturated value; fixed has no upper
bound.) Suggest checking the error:
```go
n, err := strconv.Atoi(matches[1])
if err != nil {
return fmt.Errorf("%w: %s: %w", ErrInvalidTypeString, typename, err)
}
```
##########
literals.go:
##########
@@ -1119,7 +1119,7 @@ func (b *BinaryLiteral) UnmarshalBinary(data []byte)
error {
type FixedLiteral []byte
func (FixedLiteral) Comparator() Comparator[[]byte] { return bytes.Compare }
-func (f FixedLiteral) Type() Type { return
FixedTypeOf(len(f)) }
+func (f FixedLiteral) Type() Type { return
FixedTypeOf(max(1, len(f))) }
Review Comment:
nit: unlike `DecimalLiteral.Type()` just below (which documents its
placeholder precision), this `max(1, len(f))` has no comment explaining why an
empty `FixedLiteral` reports `fixed[1]` (to avoid the new `FixedTypeOf` panic
on length 0). A one-line comment would help the next reader.
##########
literals.go:
##########
@@ -1246,12 +1246,16 @@ 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.
-func (d DecimalLiteral) Type() Type { return DecimalTypeOf(9, d.Scale) }
+// 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 {
+ precision := max(9, d.Scale)
Review Comment:
[Minor] `max(9, d.Scale)` guards the lower precision bound but not the upper
one: if `d.Scale > 38`, `precision` becomes `> 38` and the now-validating
`DecimalTypeOf` panics - whereas the old code returned a placeholder without
panicking. `FixedLiteral.Type()` is fully guarded by `max(1, len(f))`, so this
is an asymmetry. If `Scale` can exceed 38, consider clamping (e.g. `precision
:= min(maxDecimalPrecision, max(9, d.Scale))`) or confirm scale is always <= 38.
--
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]
