zeroshade commented on code in PR #2129:
URL: https://github.com/apache/iceberg-go/pull/2129#discussion_r4198584304
##########
transforms_test.go:
##########
@@ -1162,6 +1163,57 @@ func TestTruncateTransform(t *testing.T) {
}
}
+func TestTruncateTransformDecimal(t *testing.T) {
+ t.Parallel()
+
+ values := []string{
+ "0", "1", "-1", "49", "-49", "50", "-50", "51", "-51",
+ "9223372036854775808", "-9223372036854775808",
+ "18446744073709551616", "-18446744073709551616",
+ "99999999999999999999999999999999999999",
+ "-99999999999999999999999999999999999999",
+ }
+ for _, width := range []int{1, 2, 3, 10, 50, 97, math.MaxInt32} {
+ transform := iceberg.TruncateTransform{Width: width}
+ for _, scale := range []int{0, 2, 9, 38} {
+ transformer, err :=
transform.Transformer(iceberg.DecimalTypeOf(38, scale))
+ require.NoError(t, err)
+ for _, value := range values {
+ t.Run(fmt.Sprintf("width=%d/scale=%d/value=%s",
width, scale, value), func(t *testing.T) {
+ t.Parallel()
+
+ unscaled, ok :=
new(big.Int).SetString(value, 10)
+ require.True(t, ok)
+ input := iceberg.Decimal{Val:
decimal128.FromBigInt(unscaled), Scale: scale}
+ divisor := big.NewInt(int64(width))
+ quotient := new(big.Int).Div(unscaled,
divisor)
+ expected := iceberg.Decimal{
+ Val:
decimal128.FromBigInt(quotient.Mul(quotient, divisor)),
+ Scale: scale,
+ }
+
+ assert.Equal(t, expected,
transformer(input))
+ result :=
transform.Apply(iceberg.Optional[iceberg.Literal]{
+ Val:
iceberg.DecimalLiteral(input),
+ Valid: true,
+ })
+ require.True(t, result.Valid)
+ assert.Equal(t,
iceberg.DecimalLiteral(expected), result.Val)
+ assert.Equal(t, value,
input.Val.BigInt().String())
Review Comment:
nit: a transform bug can't make this fail. `iceberg.Decimal` /
`decimal128.Num` are passed by value, so neither `transformer(input)` nor
`Apply` can modify `input`. The line only re-checks that
`decimal128.FromBigInt` round-trips the fixture string. I'd drop it.
##########
transforms.go:
##########
@@ -784,9 +784,8 @@ func (t TruncateTransform) Transformer(src Type) (func(any)
any, error) {
val := v.(Decimal)
unscaled := val.Val.BigInt()
- // unscaled - (((unscaled % width) + width) % width)
+ // big.Int.Mod already gives a non-negative remainder
for positive widths.
Review Comment:
nit: the int32/int64 branches above still spell out the spec formula, so a
reader comparing the three branches has to work out why decimal differs. It's
worth keeping the reference and saying why one `Mod` is enough:
```suggestion
// Spec: v - (((v % W) + W) % W). big.Int.Mod is
Euclidean, so for
// W > 0 (see validateWidth) it already equals ((v % W)
+ W) % W.
```
--
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]
