From 531d60d07f0b1bd3d43650292d2ddccc14473af5 Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Mon, 23 Oct 2023 17:47:36 -0400 Subject: [PATCH 1/5] GH-38395: [Go] fix rounding errors in decimal256 string functions --- go/arrow/decimal256/decimal256.go | 4 ++-- go/arrow/decimal256/decimal256_test.go | 32 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/go/arrow/decimal256/decimal256.go b/go/arrow/decimal256/decimal256.go index 4bfcd4e0419f2..766387a023fd4 100644 --- a/go/arrow/decimal256/decimal256.go +++ b/go/arrow/decimal256/decimal256.go @@ -154,7 +154,7 @@ func FromString(v string, prec, scale int32) (n Num, err error) { return } - out.Mul(out, big.NewFloat(math.Pow10(int(scale)))).SetPrec(precInBits) + out.Mul(out, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())).SetPrec(precInBits) // Since we're going to truncate this to get an integer, we need to round // the value instead because of edge cases so that we match how other implementations // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if @@ -506,7 +506,7 @@ func (n Num) FitsInPrecision(prec int32) bool { func (n Num) ToString(scale int32) string { f := (&big.Float{}).SetInt(n.BigInt()) - f.Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) + f.SetPrec(256).Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) return f.Text('f', int(scale)) } diff --git a/go/arrow/decimal256/decimal256_test.go b/go/arrow/decimal256/decimal256_test.go index b944d0b1305df..2c7e8a0e4c4ee 100644 --- a/go/arrow/decimal256/decimal256_test.go +++ b/go/arrow/decimal256/decimal256_test.go @@ -20,6 +20,7 @@ import ( "fmt" "math" "math/big" + "strings" "testing" "github.com/apache/arrow/go/v14/arrow/decimal256" @@ -575,3 +576,34 @@ func TestFromString(t *testing.T) { }) } } + +// Test issues from GH-38395 +func TestToString(t *testing.T) { + const decStr = "3379334159166193114608287418738414931564221155305735605033949613740461239999" + + integer, _ := (&big.Int{}).SetString(decStr, 10) + dec := decimal256.FromBigInt(integer) + + expected := "0." + decStr + actual := dec.ToString(76) + + if expected != actual { + t.Errorf("expected: %s, actual: %s\n", expected, actual) + } +} + +// Test issues from GH-38395 +func TestHexFromString(t *testing.T) { + const decStr = "11111111111111111111111111111111111111.00000000000000000000000000000000000000" + + num, err := decimal256.FromString(decStr, 76, 38) + if err != nil { + t.Error(err) + } else if decStr != num.ToString(38) { + t.Errorf("expected: %s, actual: %s\n", decStr, num.ToString(38)) + + actualCoeff := num.BigInt() + expectedCoeff, _ := (&big.Int{}).SetString(strings.Replace(decStr, ".", "", -1), 10) + t.Errorf("expected(hex): %X, actual(hex): %X\n", expectedCoeff.Bytes(), actualCoeff.Bytes()) + } +} From 87263cb15a365d471e60b0ca35c275d44cb44e1c Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 24 Oct 2023 11:51:52 -0400 Subject: [PATCH 2/5] fix negative scale support --- go/arrow/decimal128/decimal128.go | 41 +++++++++++++++++--------- go/arrow/decimal128/decimal128_test.go | 1 + go/arrow/decimal256/decimal256.go | 39 +++++++++++++++--------- go/arrow/decimal256/decimal256_test.go | 1 + 4 files changed, 54 insertions(+), 28 deletions(-) diff --git a/go/arrow/decimal128/decimal128.go b/go/arrow/decimal128/decimal128.go index 898d7b427ec84..020042a8a6cb6 100644 --- a/go/arrow/decimal128/decimal128.go +++ b/go/arrow/decimal128/decimal128.go @@ -266,23 +266,36 @@ func FromString(v string, prec, scale int32) (n Num, err error) { return } - // Since we're going to truncate this to get an integer, we need to round - // the value instead because of edge cases so that we match how other implementations - // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if - // we're positive we'll add 0.5. - out.Mul(out, big.NewFloat(math.Pow10(int(scale)))).SetPrec(precInBits) - if out.Signbit() { - out.Sub(out, pt5) + if scale < 0 { + var tmp big.Int + val, _ := out.Int(&tmp) + if val.BitLen() > 127 { + return Num{}, errors.New("bitlen too large for decimal128") + } + n = FromBigInt(val) + n, _ = n.Div(scaleMultipliers[-scale]) } else { - out.Add(out, pt5) - } - var tmp big.Int - val, _ := out.Int(&tmp) - if val.BitLen() > 127 { - return Num{}, errors.New("bitlen too large for decimal128") + // Since we're going to truncate this to get an integer, we need to round + // the value instead because of edge cases so that we match how other implementations + // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if + // we're positive we'll add 0.5. + p := (&big.Float{}).SetFloat64(float64PowersOfTen[scale+38]) + out.Mul(out, p).SetPrec(precInBits) + if out.Signbit() { + out.Sub(out, pt5) + } else { + out.Add(out, pt5) + } + + var tmp big.Int + val, _ := out.Int(&tmp) + if val.BitLen() > 127 { + return Num{}, errors.New("bitlen too large for decimal128") + } + n = FromBigInt(val) } - n = FromBigInt(val) + if !n.FitsInPrecision(prec) { err = fmt.Errorf("val %v doesn't fit in precision %d", n, prec) } diff --git a/go/arrow/decimal128/decimal128_test.go b/go/arrow/decimal128/decimal128_test.go index 6fa54091019e7..8665b4a59df8b 100644 --- a/go/arrow/decimal128/decimal128_test.go +++ b/go/arrow/decimal128/decimal128_test.go @@ -659,6 +659,7 @@ func TestFromString(t *testing.T) { {"1e-37", 1, 37}, {"2112.33", 211233, 2}, {"-2112.33", -211233, 2}, + {"12E2", 12, -2}, } for _, tt := range tests { diff --git a/go/arrow/decimal256/decimal256.go b/go/arrow/decimal256/decimal256.go index 766387a023fd4..e2a88b277139b 100644 --- a/go/arrow/decimal256/decimal256.go +++ b/go/arrow/decimal256/decimal256.go @@ -154,23 +154,34 @@ func FromString(v string, prec, scale int32) (n Num, err error) { return } - out.Mul(out, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())).SetPrec(precInBits) - // Since we're going to truncate this to get an integer, we need to round - // the value instead because of edge cases so that we match how other implementations - // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if - // we're positive we'll add 0.5. - if out.Signbit() { - out.Sub(out, pt5) + if scale < 0 { + var tmp big.Int + val, _ := out.Int(&tmp) + if val.BitLen() > 255 { + return Num{}, errors.New("bitlen too large for decimal256") + } + n = FromBigInt(val) + + n, _ = n.Div(scaleMultipliers[-scale]) } else { - out.Add(out, pt5) - } + out.Mul(out, (&big.Float{}).SetFloat64(float64PowersOfTen[scale+76])).SetPrec(precInBits) + // Since we're going to truncate this to get an integer, we need to round + // the value instead because of edge cases so that we match how other implementations + // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if + // we're positive we'll add 0.5. + if out.Signbit() { + out.Sub(out, pt5) + } else { + out.Add(out, pt5) + } - var tmp big.Int - val, _ := out.Int(&tmp) - if val.BitLen() > 255 { - return Num{}, errors.New("bitlen too large for decimal256") + var tmp big.Int + val, _ := out.Int(&tmp) + if val.BitLen() > 255 { + return Num{}, errors.New("bitlen too large for decimal256") + } + n = FromBigInt(val) } - n = FromBigInt(val) if !n.FitsInPrecision(prec) { err = fmt.Errorf("value %v doesn't fit in precision %d", n, prec) } diff --git a/go/arrow/decimal256/decimal256_test.go b/go/arrow/decimal256/decimal256_test.go index 2c7e8a0e4c4ee..f4c6cb49cf18e 100644 --- a/go/arrow/decimal256/decimal256_test.go +++ b/go/arrow/decimal256/decimal256_test.go @@ -564,6 +564,7 @@ func TestFromString(t *testing.T) { {"1e-37", 1, 37}, {"2112.33", 211233, 2}, {"-2112.33", -211233, 2}, + {"12E2", 12, -2}, } for _, tt := range tests { From ea06abe662004334144fe30976edaaa42c916062 Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 24 Oct 2023 12:04:08 -0400 Subject: [PATCH 3/5] negative scale ToString --- go/arrow/decimal128/decimal128.go | 6 +++++- go/arrow/decimal256/decimal256.go | 6 +++++- go/arrow/decimal256/decimal256_test.go | 7 ++----- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/go/arrow/decimal128/decimal128.go b/go/arrow/decimal128/decimal128.go index 020042a8a6cb6..62249789f57fd 100644 --- a/go/arrow/decimal128/decimal128.go +++ b/go/arrow/decimal128/decimal128.go @@ -518,7 +518,11 @@ func (n Num) FitsInPrecision(prec int32) bool { func (n Num) ToString(scale int32) string { f := (&big.Float{}).SetInt(n.BigInt()) - f.Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) + if scale < 0 { + f.SetPrec(128).Mul(f, (&big.Float{}).SetInt(scaleMultipliers[-scale].BigInt())) + } else { + f.SetPrec(128).Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) + } return f.Text('f', int(scale)) } diff --git a/go/arrow/decimal256/decimal256.go b/go/arrow/decimal256/decimal256.go index e2a88b277139b..88f108c76da8d 100644 --- a/go/arrow/decimal256/decimal256.go +++ b/go/arrow/decimal256/decimal256.go @@ -517,7 +517,11 @@ func (n Num) FitsInPrecision(prec int32) bool { func (n Num) ToString(scale int32) string { f := (&big.Float{}).SetInt(n.BigInt()) - f.SetPrec(256).Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) + if scale < 0 { + f.SetPrec(256).Mul(f, (&big.Float{}).SetInt(scaleMultipliers[-scale].BigInt())) + } else { + f.SetPrec(256).Quo(f, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())) + } return f.Text('f', int(scale)) } diff --git a/go/arrow/decimal256/decimal256_test.go b/go/arrow/decimal256/decimal256_test.go index f4c6cb49cf18e..047867f3433ca 100644 --- a/go/arrow/decimal256/decimal256_test.go +++ b/go/arrow/decimal256/decimal256_test.go @@ -586,11 +586,8 @@ func TestToString(t *testing.T) { dec := decimal256.FromBigInt(integer) expected := "0." + decStr - actual := dec.ToString(76) - - if expected != actual { - t.Errorf("expected: %s, actual: %s\n", expected, actual) - } + assert.Equal(t, expected, dec.ToString(76)) + assert.Equal(t, decStr+"0000", dec.ToString(-4)) } // Test issues from GH-38395 From f9ea3ff6479194eda83aef831618179b8ca62e64 Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 24 Oct 2023 12:08:22 -0400 Subject: [PATCH 4/5] i hate fp precision --- go/arrow/decimal128/decimal128.go | 3 +-- go/arrow/decimal256/decimal256.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/go/arrow/decimal128/decimal128.go b/go/arrow/decimal128/decimal128.go index 62249789f57fd..ff9c088b9d638 100644 --- a/go/arrow/decimal128/decimal128.go +++ b/go/arrow/decimal128/decimal128.go @@ -275,12 +275,11 @@ func FromString(v string, prec, scale int32) (n Num, err error) { n = FromBigInt(val) n, _ = n.Div(scaleMultipliers[-scale]) } else { - // Since we're going to truncate this to get an integer, we need to round // the value instead because of edge cases so that we match how other implementations // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if // we're positive we'll add 0.5. - p := (&big.Float{}).SetFloat64(float64PowersOfTen[scale+38]) + p := (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt()) out.Mul(out, p).SetPrec(precInBits) if out.Signbit() { out.Sub(out, pt5) diff --git a/go/arrow/decimal256/decimal256.go b/go/arrow/decimal256/decimal256.go index 88f108c76da8d..cf2157fdd48ed 100644 --- a/go/arrow/decimal256/decimal256.go +++ b/go/arrow/decimal256/decimal256.go @@ -164,7 +164,7 @@ func FromString(v string, prec, scale int32) (n Num, err error) { n, _ = n.Div(scaleMultipliers[-scale]) } else { - out.Mul(out, (&big.Float{}).SetFloat64(float64PowersOfTen[scale+76])).SetPrec(precInBits) + out.Mul(out, (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())).SetPrec(precInBits) // Since we're going to truncate this to get an integer, we need to round // the value instead because of edge cases so that we match how other implementations // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if From 0cf5021bbc8c153525cfcec79b0b983dd8ccc367 Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 24 Oct 2023 16:17:06 -0400 Subject: [PATCH 5/5] updates from feedback --- go/arrow/decimal128/decimal128_test.go | 16 ++++++++++++++++ go/arrow/decimal256/decimal256_test.go | 18 +++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/go/arrow/decimal128/decimal128_test.go b/go/arrow/decimal128/decimal128_test.go index 8665b4a59df8b..102eede6d3b28 100644 --- a/go/arrow/decimal128/decimal128_test.go +++ b/go/arrow/decimal128/decimal128_test.go @@ -682,3 +682,19 @@ func TestInvalidNonNegScaleFromString(t *testing.T) { }) } } + +func TestBitLen(t *testing.T) { + n := decimal128.GetScaleMultiplier(38) + b := n.BigInt() + b.Mul(b, big.NewInt(25)) + assert.Greater(t, b.BitLen(), 128) + + assert.Panics(t, func() { + decimal128.FromBigInt(b) + }) + + _, err := decimal128.FromString(b.String(), decimal128.MaxPrecision, 0) + assert.ErrorContains(t, err, "bitlen too large for decimal128") + _, err = decimal128.FromString(b.String(), decimal128.MaxPrecision, -1) + assert.ErrorContains(t, err, "bitlen too large for decimal128") +} diff --git a/go/arrow/decimal256/decimal256_test.go b/go/arrow/decimal256/decimal256_test.go index 047867f3433ca..9c734e23aa0d6 100644 --- a/go/arrow/decimal256/decimal256_test.go +++ b/go/arrow/decimal256/decimal256_test.go @@ -586,7 +586,7 @@ func TestToString(t *testing.T) { dec := decimal256.FromBigInt(integer) expected := "0." + decStr - assert.Equal(t, expected, dec.ToString(76)) + assert.Equal(t, expected, dec.ToString(int32(len(decStr)))) assert.Equal(t, decStr+"0000", dec.ToString(-4)) } @@ -605,3 +605,19 @@ func TestHexFromString(t *testing.T) { t.Errorf("expected(hex): %X, actual(hex): %X\n", expectedCoeff.Bytes(), actualCoeff.Bytes()) } } + +func TestBitLen(t *testing.T) { + n := decimal256.GetScaleMultiplier(76) + b := n.BigInt() + b.Mul(b, big.NewInt(25)) + assert.Greater(t, b.BitLen(), 255) + + assert.Panics(t, func() { + decimal256.FromBigInt(b) + }) + + _, err := decimal256.FromString(b.String(), decimal256.MaxPrecision, 0) + assert.ErrorContains(t, err, "bitlen too large for decimal256") + _, err = decimal256.FromString(b.String(), decimal256.MaxPrecision, -1) + assert.ErrorContains(t, err, "bitlen too large for decimal256") +}