sql: disable ordering by array columns

[justinj]

In accordance with #16172, we will need to disallow ordering by arrays
columns to keep open the door to decide on how we should order them
later on.

I debated making the check a method on the Type interface, but I decided
that was overkill for this single check on a single Type. Regardless,
the check is isolated into a single function so it can be changed
easily. This approach doesn't play well with wrapped Oid types like
INT2VECTOR and INT4VECTOR, but I'm not too concerned about those because
users can't construct them, and since they only exist for pg_catalog
compatibility anyway we might as well allow ordering by them as in
Postgres.

We currently don't support IN or MAX/MIN on arrays, so that's not
something we need to be concerned about.

As far as I'm aware there are no other situations where the ordering of
arrays is exposed to users. I tried adding a panic to TArray's Compare
method and the only test failure was a test calling Compare explicitly
as an assertion.

This is technically a breaking change which should be announced to users
once it is released (though I doubt anyone is making use of this).

Also updated the array rfc to be in-progress.

[cockroach-teamcity]

This change is 

[jordanlewis]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/sort.go, line 172 at r1 (raw file):

		}

		// So Then, deal with column ordinals.

I'm not too familiar with column ordinals, but it seems like you need to check that they're all comparable too, right?

---

Comments from Reviewable

[justinj]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/sort.go, line 172 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I'm not too familiar with column ordinals, but it seems like you need to check that they're all comparable too, right?

Both this case and the one above are handled by the block on L185, they both just set index if they find a column they can order by, which that block then checks. The general structure of this function took me a while to grok, but it roughly goes like this:

1. see if we're ordering by any of the rendered expressions, if we are, set index to that column
2. if we haven't set index (that is, 1 failed) see if we're ordering by a column ordinal, if we are, set index to that
3. if we haven't set index (that is, 1 and 2 failed), then add a new render target for the expression we're ordering by

The check on L185 handles both cases 1 and 2, the check on L219 handles case 3. The reason the two checks are different is that cases 1 and 2 use an existing column while case 3 uses a new one.

This function could maybe do with some function extraction to make the above structure more obvious.

---

Comments from Reviewable

[jordanlewis]

once you're satisfied with the test coverage

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/sort.go, line 172 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Both this case and the one above are handled by the block on L185, they both just set index if they find a column they can order by, which that block then checks. The general structure of this function took me a while to grok, but it roughly goes like this:

1. see if we're ordering by any of the rendered expressions, if we are, set index to that column
2. if we haven't set index (that is, 1 failed) see if we're ordering by a column ordinal, if we are, set index to that
3. if we haven't set index (that is, 1 and 2 failed), then add a new render target for the expression we're ordering by

The check on L185 handles both cases 1 and 2, the check on L219 handles case 3. The reason the two checks are different is that cases 1 and 2 use an existing column while case 3 uses a new one.

This function could maybe do with some function extraction to make the above structure more obvious.

Gotcha. Do your tests exercise both of these cases?

---

Comments from Reviewable

[justinj]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/sort.go, line 172 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Gotcha. Do your tests exercise both of these cases?

Yup, the query error cases I added handle the four main cases (new expression, same expression, column ordinal, aliased expression).

---

Comments from Reviewable

[knz]

Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[knz]

Reviewed 2 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable