opt, sqlsmith: fix incorrect output with NULLS LAST in window and aggregate functions

[rytaft]

opt: fix incorrect output with NULLS LAST in window and aggregate functions

Prior to this commit, window and aggregate functions dropped any NULLS LAST
modifier on their order by clauses. This commit updates the optbuilder to
add an implicit col IS NULL ordering column for each column col that uses
NULLS LAST.

Fixes #91295

Release note (bug fix): Fixed a bug in which the non-default nulls ordering,
NULLS LAST, was ignored in window and aggregate functions. This bug could cause
incorrect query results when NULLS LAST was used, and was introduced in 22.1.0.
This is now fixed.

sqlsmith: added support for randomly using NULLS FIRST or NULLS LAST

Prior to this commit, SQLSmith did not produce queries that used the
non-default nulls ordering. This commit adds support for sometimes using
NULLS FIRST or NULLS LAST with ordering clauses.

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

LGTM

Reviewed 4 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2)

[rytaft]

I updated the code to actually fix the bug, since returning an error won't work due to the null_ordered_last session setting. I think it's still backportable. PTAL. Thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

[michae2]

Thanks for fixing it! I had some questions, but in general it

Reviewed 1 of 4 files at r1, 8 of 8 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft)

---

pkg/sql/opt/optbuilder/groupby.go line 728 at r3 (raw file):

			nullsDefaultOrder := b.hasDefaultNullsOrder(o)
			if !nullsDefaultOrder {
				expr := tree.NewTypedIsNullExpr(o.Expr.(tree.TypedExpr))

Do we need to do something special here to handle the case where expr is a tuple, like we did in window functions?

---

pkg/sql/opt/optbuilder/orderby.go line 287 at r3 (raw file):

			((order.NullsOrder == tree.NullsFirst && order.Direction == tree.Descending) ||
				(order.NullsOrder == tree.NullsLast && order.Direction != tree.Descending))) {
		telemetry.Inc(sqltelemetry.OrderByNullsNonStandardCounter)

Side note, I know this was already here but it seems a little odd that there's nothing to compare it to (i.e. I would expect an equivalent counter for when the default nulls order is used).

---

pkg/sql/opt/optbuilder/window.go line 434 at r3 (raw file):

					// in other expressions.
					colName := scopeColName("").WithMetadataName(
						fmt.Sprintf("%s_%d_nulls_ordering_%d", funcName, windowIndex+1, j+1),

Should this also use the index of e within cols in case this is a tuple with multiple columns that cannot be referenced?

[rytaft]

TFTR! I went down a very deep rabbit hole trying to understand Postgres' behavior with NULLS FIRST/LAST and tuples, and I realized that our current approach of adding an extra ordering column in the optimizer unfortunately just doesn't work very well with tuples (i.e., we'll never be able to completely match Postgres' behavior with the current approach). I opened #93558 to track that issue, but it seems difficult to fix and much lower priority than #91295 (which this PR fixes).

So even if our behavior is somewhat wrong for tuples, at least we are now internally consistent. And this PR should fix the non-tuple cases completely.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @michae2)

---

pkg/sql/opt/optbuilder/groupby.go line 728 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Do we need to do something special here to handle the case where expr is a tuple, like we did in window functions?

Great catch! This was actually a pre-existing bug that caused expressions like array_agg((k, v) ORDER BY (k+1, v||'foo')) to fail. Added a test for this too.

---

pkg/sql/opt/optbuilder/orderby.go line 287 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Side note, I know this was already here but it seems a little odd that there's nothing to compare it to (i.e. I would expect an equivalent counter for when the default nulls order is used).

Done.

---

pkg/sql/opt/optbuilder/window.go line 434 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should this also use the index of e within cols in case this is a tuple with multiple columns that cannot be referenced?

Done. (I don't think it matters that the metadata name is unique, but fixed just in case...)

[michae2]

Nice!

That slack thread definitely taught me a thing or two! 😆

Reviewed 5 of 7 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft)

---

pkg/sql/opt/optbuilder/groupby.go line 728 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Great catch! This was actually a pre-existing bug that caused expressions like array_agg((k, v) ORDER BY (k+1, v||'foo')) to fail. Added a test for this too.

Wow! I was not expecting that, haha. SQL is ridiculous sometimes.

---

pkg/sql/opt/optbuilder/orderby.go line 287 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Done.

Thank you!

[rytaft]

Me too!

bors r+

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

[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 b10363d to blathers/backport-release-22.1-93426: 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.1.x failed. See errors above.

---

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