sql: fix decimal evaluation edge cases

[DrewKimball]

Previously, the logic for decimal and float division, floor division and mod operators was incorrect for a few edge cases involving NaN or Infinity values. For example, 'NaN'::DECIMAL / 0 would throw a division-by-zero error when it should evaluate to NaN and 0/'inf'::DECIMAL returned 0E-2019 instead of just 0.

This patch updates the special-case logic to mirror that of postgres, so division-by-zero errors always check the NaN case and the division by infinity case returns a 0 without extra digits.

Fixes #40929
Fixes #103633

Release note (bug fix): Fixed edge cases in decimal and float evaluation for division operators. 'NaN'::DECIMAL / 0 will now return NaN instead of a division-by-zero error, and 0 / 'inf'::DECIMAL will return 0 instead of 0E-2019.

[cockroach-teamcity]

This change is 

[yuzefovich]

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

-- commits line 14 at r1:
Nice! We probably should've fixed it long time ago 😅 You should also adjust sqlsmith roachtest to remove the skip for the error due to #40929.

---

pkg/sql/sem/tree/datum.go line 1168 at r1 (raw file):

	// DNaNDecimal is the decimal constant 'NaN'.
	DNaNDecimal = &DDecimal{Decimal: apd.Decimal{Form: apd.NaN}}

nit: it doesn't look like DNaNDecimal and DPosInfDecimal are used in other packages, so maybe not export them?

---

pkg/sql/logictest/testdata/logic_test/decimal line 484 at r1 (raw file):

NaN        Infinity   NaN                      NaN   NaN
-Infinity  NaN        NaN                      NaN   NaN
-Infinity  -Infinity  0                        NaN   NaN

There are some minor differences from postgres in this table. I think Inf % Inf in PG returns NaN whereas we now return 0. It'd be good to adjust these queries a bit to add ORDER BY x1, x2 and compare so that we match PG exactly.

---

pkg/sql/logictest/testdata/logic_test/decimal line 627 at r1 (raw file):

# infinity. The results should only have one digit each.
statement ok
CREATE TABLE regression_40929 AS SELECT g FROM generate_series(1, 1) AS g;

nit: since we have generate_series(1, 1) we could simply it to just VALUES (1), right?

[DrewKimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

-- commits line 14 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Nice! We probably should've fixed it long time ago 😅 You should also adjust sqlsmith roachtest to remove the skip for the error due to #40929.

Good catch, Done.

---

pkg/sql/sem/tree/datum.go line 1168 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: it doesn't look like DNaNDecimal and DPosInfDecimal are used in other packages, so maybe not export them?

Done.

---

pkg/sql/logictest/testdata/logic_test/decimal line 484 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

There are some minor differences from postgres in this table. I think Inf % Inf in PG returns NaN whereas we now return 0. It'd be good to adjust these queries a bit to add ORDER BY x1, x2 and compare so that we match PG exactly.

Oh, good catch - the infinity checks should check that the left side is Finite, not that it isn't NaN. I added an id column to ensure the ordering, since postgres is different for both NULL and NaN.

---

pkg/sql/logictest/testdata/logic_test/decimal line 627 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: since we have generate_series(1, 1) we could simply it to just VALUES (1), right?

Done.

[yuzefovich]

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

[yuzefovich]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @herkolategan, and @renatolabs)

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go line 343 at r3 (raw file):

					colexecerror.ExpectedError(err)
				}
        {{if .CheckRightIsInf}}

nit: the indentation is a bit off.

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go line 599 at r3 (raw file):

					colexecerror.ExpectedError(err)
				}
        {{if .CheckRightIsInf}}

nit: ditto.

[DrewKimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @renatolabs, and @yuzefovich)

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go line 343 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: the indentation is a bit off.

Done.

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go line 599 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: ditto.

Done.

[DrewKimball]

Failure is a known flake: #106579

[DrewKimball]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from a8b42c9 to blathers/backport-release-22.2-106472: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

error creating merge commit from a8b42c9 to blathers/backport-release-23.1-106472: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}