laskoviymishka commented on code in PR #1405:
URL: https://github.com/apache/iceberg-go/pull/1405#discussion_r3536300587
##########
types.go:
##########
@@ -578,9 +588,30 @@ func (f FixedType) String() string { return
fmt.Sprintf("fixed[%d]", f.len) }
func (f FixedType) primitive() {}
func DecimalTypeOf(prec, scale int) DecimalType {
+ if err := validateDecimalPrecisionScale(prec, scale); err != nil {
Review Comment:
Panicking here makes `DecimalTypeOf` unsafe on data-derived input.
`arrow_utils.go` calls `DecimalTypeOf(dec.GetPrecision(), dec.GetScale())` for
incoming Arrow schemas, and Arrow permits scale > precision
(`Decimal32Type{Precision:8, Scale:9}` is valid), so reading such a batch now
takes down the process instead of returning an error. The `Scale:9→Scale:2`
edit in `arrow_utils_test.go` is the tell that this path is live.
I'd have `DecimalTypeOf` return `(DecimalType, error)` so `arrow_utils.go`
can propagate, rather than validate-by-panic on library input.
##########
types.go:
##########
@@ -578,9 +588,30 @@ func (f FixedType) String() string { return
fmt.Sprintf("fixed[%d]", f.len) }
func (f FixedType) primitive() {}
func DecimalTypeOf(prec, scale int) DecimalType {
+ if err := validateDecimalPrecisionScale(prec, scale); err != nil {
+ panic(fmt.Errorf("%w: %w", ErrInvalidArgument, err))
+ }
+
return DecimalType{precision: prec, scale: scale}
}
+func validateDecimalPrecisionScale(precision, scale int) error {
+ if precision <= 0 {
+ return fmt.Errorf("invalid precision %d: must be greater than
0", precision)
+ }
+ if precision > 38 {
+ return fmt.Errorf("invalid precision %d: must be less than or
equal to 38", precision)
+ }
+ if scale < 0 {
+ return fmt.Errorf("invalid scale %d: must be greater than or
equal to 0", scale)
+ }
+ if scale > precision {
Review Comment:
I don't think we can enforce `scale > precision` here. The spec only
constrains precision (must be 38 or less) — there's no rule tying scale to
precision, and Java's `Types.DecimalType`, iceberg-rust, and PyIceberg all
accept things like `decimal(5,7)`.
Once this lands, a table written by Java/Spark with scale > precision
serializes fine but our `UnmarshalJSON` rejects it, so the table becomes
unreadable here even read-only. I'd drop this guard and keep just precision in
`(0,38]` and `scale >= 0`. wdyt?
##########
types_test.go:
##########
@@ -86,6 +86,57 @@ func TestDecimalType(t *testing.T) {
assert.Equal(t, "decimal(9, 2)", typ.String())
assert.True(t, typ.Equals(iceberg.DecimalTypeOf(9, 2)))
assert.False(t, typ.Equals(iceberg.DecimalTypeOf(9, 3)))
+
+ t.Run("DecimalTypeOf validates precision and scale", func(t *testing.T)
{
+ assert.PanicsWithError(t, "invalid argument: invalid precision
0: must be greater than 0", func() {
+ iceberg.DecimalTypeOf(0, 2)
+ })
+ assert.PanicsWithError(t, "invalid argument: invalid precision
39: must be less than or equal to 38", func() {
+ iceberg.DecimalTypeOf(39, 0)
+ })
+ assert.PanicsWithError(t, "invalid argument: invalid scale 11:
must be less than or equal to precision 10", func() {
+ iceberg.DecimalTypeOf(10, 11)
+ })
+ assert.PanicsWithError(t, "invalid argument: invalid scale -1:
must be greater than or equal to 0", func() {
+ iceberg.DecimalTypeOf(10, -1)
+ })
+ })
+}
+
+func TestDecimalTypeInvalidParse(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {
+ name: "trailing chars",
+ data: `{"id": 1, "name": "d", "type":
"decimal(10,2)junk", "required": true}`,
+ },
+ {
+ name: "precision zero",
+ data: `{"id": 1, "name": "d", "type": "decimal(0,2)",
"required": true}`,
+ },
+ {
+ name: "scale exceeds precision",
+ data: `{"id": 1, "name": "d", "type": "decimal(10,11)",
"required": true}`,
+ },
+ {
+ name: "negative scale",
+ data: `{"id": 1, "name": "d", "type": "decimal(10,-1)",
"required": true}`,
Review Comment:
This case doesn't actually reach `validateDecimalPrecisionScale` — the
anchored `\d+` in `decimalRegex` rejects the `-` first, so it passes via the
regex mismatch, not the `scale < 0` branch. That branch is only reachable
through `DecimalTypeOf`, which this test doesn't touch.
Worth a note, or point it at `DecimalTypeOf(10, -1)` if you want to cover
the branch it's named for.
##########
types.go:
##########
@@ -197,8 +197,18 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- prec, _ := strconv.Atoi(matches[1])
- scale, _ := strconv.Atoi(matches[2])
+ 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: %s",
ErrInvalidTypeString, err)
Review Comment:
Small thing: this interpolates a real error with `%s`, which drops its wrap
chain — callers can't `errors.As`/`Is` back to the cause. Since Go 1.20
supports multiple `%w` (and the `DecimalTypeOf` panic just above already uses
double-`%w`), I'd make this `%w: %w`.
##########
literals_test.go:
##########
@@ -389,6 +389,15 @@ func TestDecimalToDecimalConversion(t *testing.T) {
assert.ErrorContains(t, err, "could not convert 34.11 to decimal(9, 3)")
}
+func TestDecimalLiteralTypeSupportsLargeScale(t *testing.T) {
+ lit := iceberg.DecimalLiteral{Scale: 10, Val: decimal128.FromI64(1234)}
+
+ assert.NotPanics(t, func() {
+ typeStr := lit.Type().String()
+ assert.Equal(t, "decimal(9, 10)", typeStr)
Review Comment:
This pins `decimal(9, 10)` — an invalid type — as a guarantee, but the name
frames it as a feature. If #1028 ever gets fixed and `Type()` stops hardcoding
precision 9, this test breaks even though the fix is correct.
I'd either drop the exact-string assert or rename it as a known-wart
characterization and reference #1028 in the body, so a future maintainer knows
it's intentional.
--
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]
