Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Go] rounding errors in decimal256 string functions #38395

Closed
kuerbiskarton opened this issue Oct 23, 2023 · 1 comment · Fixed by #38426
Closed

[Go] rounding errors in decimal256 string functions #38395

kuerbiskarton opened this issue Oct 23, 2023 · 1 comment · Fixed by #38426
Assignees
Milestone

Comments

@kuerbiskarton
Copy link

kuerbiskarton commented Oct 23, 2023

There are rounding errors when constructing decimal256 values from string and when serializing them to string.

Affected version: main branch, v13 (maybe even older versions).

Similar issues were already reported in the past (#35310).

func TestFromString(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())
	}
}

fails with

expected: 11111111111111111111111111111111111111.00000000000000000000000000000000000000
actual:   11111111111111110860978869272892669951.88888888888888889139021130727107330048

expected(hex): 0274DDD9AC9F3B4D00D24CF6A80B3D7F3DCD982490C8B30EE27EDFC000000000
actual(hex):   0274DDD9AC9F3B4904B40E1450CD9B8435D513ED4B465C000000000000000000

The hex numbers show that the error is in the FromString function.

However, there are also rounding issues in the ToString function:

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)
	}
}

fails with

expected: 0.3379334159166193114608287418738414931564221155305735605033949613740461239999
actual:   0.3379334159166193114608287418738414931564221155305735605033949613740461240000

In case of ToString it seem to be off-by-one errors only.

Component(s)

Go

zeroshade added a commit that referenced this issue Oct 24, 2023
…8426)

### Rationale for this change
Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale.

### Are these changes tested?
Yes

* Closes: #38395

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 15.0.0 milestone Oct 24, 2023
JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 25, 2023
…ns (apache#38426)

### Rationale for this change
Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale.

### Are these changes tested?
Yes

* Closes: apache#38395

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@kuerbiskarton
Copy link
Author

@zeroshade Thanks for the fix.

I did some testing.
decimal256.ToString, decimal128.ToString and decimal256.FromString work correctly now.
There is still an off-by-one error in decimal128.FromString:

func TestFromStringDecimal128b(t *testing.T) {
	const decStr = "766710016439641.74508380782495962772837"

	num, err := decimal128.FromString(decStr, 15+23, 23)
	if err != nil {
		t.Error(err)
	} else if decStr != num.ToString(23) {
		t.Errorf("expected: %s, actual: %s\n", decStr, num.ToString(23))

		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())
	}
}

fails with

expected: 766710016439641.74508380782495962772837
actual  : 766710016439641.74508380782495962772838
expected(hex): 39AE4C3C2D4C7A4D816B065457BC3565
actual(hex)  : 39AE4C3C2D4C7A4D816B065457BC3566

loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…ns (apache#38426)

### Rationale for this change
Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale.

### Are these changes tested?
Yes

* Closes: apache#38395

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…ns (apache#38426)

### Rationale for this change
Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale.

### Are these changes tested?
Yes

* Closes: apache#38395

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants