util/decimal: decrease allowed shift length

[madelynnblue]

Improve decimal testing to include a 5s timeout. Also use subtests for
better test messages.

Fixes #10928

---

This change is 

[knz]

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/util/decimal/decimal.go, line 337 at r1 (raw file):

	// this it becomes very slow. However since the implementation for that lies
	// in the inf package, we instead check the length in this function. The
	// tests in TestDecimalLogN that using strE (a very high-precision E value)

I think a word is missing here. Perhaps "the tests ... that are using strE" ?

---

Comments from Reviewable

[tamird]

Reviewed 1 of 2 files at r2.
Review status: 3 of 4 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

pkg/util/decimal/decimal_test.go, line 87 at r2 (raw file):

) {
	for _, tc := range tests {
		t.Run(fmt.Sprintf("%s = %s", tc.input, tc.expected), func(t *testing.T) {

it's a good idea to avoid spaces in these names. run this test with -test.v to see why.

---

pkg/util/decimal/decimal_test.go, line 105 at r2 (raw file):

				t.Logf("execute duration: %s", timeutil.Since(start))
			case <-time.After(testFuncTimeout):
				t.Fatalf("timedout after %s", testFuncTimeout)

won't this leak goroutines? whoa, is the parser package no using leaktest?

---

pkg/util/decimal/decimal_test.go, line 111 at r2 (raw file):

					t.Errorf("expected error %s, got %s", tc.expected, err)
				}
				return

not new but this seems to do the wrong thing if an error is expected and not returned.

---

pkg/util/decimal/decimal_test.go, line 157 at r2 (raw file):

) {
	for _, tc := range tests {
		t.Run(fmt.Sprintf("%s, %s = %s", tc.input1, tc.input2, tc.expected), func(t *testing.T) {

it's a good idea to avoid spaces in these names. run this test with -test.v to see why.

---

pkg/util/decimal/decimal_test.go, line 161 at r2 (raw file):

			if _, ok := x.SetString(tc.input1); !ok {
				t.Errorf("could not set decimal: %s", tc.input1)
				return

this is equivalent to Fataling on the previous line

---

pkg/util/decimal/decimal_test.go, line 165 at r2 (raw file):

			if _, ok := y.SetString(tc.input2); !ok {
				t.Errorf("could not set decimal: %s", tc.input2)
				return

this is equivalent to Fataling on the previous line

---

Comments from Reviewable

[madelynnblue]

Review status: 2 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

pkg/util/decimal/decimal.go, line 337 at r1 (raw file):

Previously, knz (kena) wrote…

> I think a word is missing here. Perhaps "the tests ... that *are* using strE" ?

Done.

---

pkg/util/decimal/decimal_test.go, line 87 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> it's a good idea to avoid spaces in these names. run this test with `-test.v` to see why.

I prefer the underscores myself, but I've changed these.

---

pkg/util/decimal/decimal_test.go, line 105 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> won't this leak goroutines? whoa, is the parser package no using leaktest?

Changed to use a timer and Stop. Should prevent leaks.

---

pkg/util/decimal/decimal_test.go, line 111 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> not new but this seems to do the wrong thing if an error is expected and not returned.

I think it's ok. If an error is expected, it will fail to set and compare during exp.SetString and exp.Cmp later down since tc.expected is an error string, not a valid decimal.

---

pkg/util/decimal/decimal_test.go, line 161 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> this is equivalent to `Fatal`ing on the previous line

Done.

---

pkg/util/decimal/decimal_test.go, line 165 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> this is equivalent to `Fatal`ing on the previous line

Done.

---

Comments from Reviewable

[madelynnblue]

PTAL

[knz]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

pkg/util/decimal/decimal_test.go, line 161 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

> Done.

I may be missing something; you have made two such substitutions below this point but this one (on line 162-163) and the following seem to remain.

---

Comments from Reviewable

[knz]

Also please check (possibly manually) that this change properly addresses the latest repro SELECT pow(2.306257620454719e+12::decimal, 49.18687811476825::decimal).

[knz]

Another repro: SELECT POW(791018.4038517432::decimal, 155.94813858582634::decimal)

[madelynnblue]

I've also added those other two failing RSG tests. They pass here.

---

Review status: 3 of 4 files reviewed at latest revision, 6 unresolved discussions.

---

pkg/util/decimal/decimal_test.go, line 161 at r2 (raw file):

Previously, knz (kena) wrote…

I may be missing something; you have made two such substitutions below this point but this one (on line 162-163) and the following seem to remain.

Yeah not sure how that didn't get picked up. I thought I already had this. Added.

---

Comments from Reviewable

[knz]

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

Comments from Reviewable