sql: simplify the subquery typing rules

[petermattis]

The simplification allows Cockroach to support a strict superset of the
queries Postgres supports.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

---

Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/subquery.go, line 404 at r2 (raw file):

if the subquery returns on a single column, use the type of that column as the type for the subquery.

How does SELECT 1 IN (SELECT 1); still work then?

---

pkg/sql/logictest/testdata/logic_test/subquery, line 114 at r2 (raw file):

----
true

Could you add the case SELECT (SELECT 1, 2) IN (SELECT 1, 2);? It's another one that we support (and MySQL supports) but PG doesn't.

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/subquery.go, line 404 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

if the subquery returns on a single column, use the type of that column as the type for the subquery.

How does SELECT 1 IN (SELECT 1); still work then?

The comment was inaccurate. In a multi-row context (such as the right-hand side of an IN expression), we always wrap the inner type U in a tuple resulting in tuple{U}. If the subquery returns a single column, the type U is the type of that column. If the subquery returns multiple columns C, the type is tuple{C}. So for the above query, the type of the subquery will be tuple{int}.

I've rewritten the comment to make it clearer what the subquery typing rules ares.

---

pkg/sql/logictest/testdata/logic_test/subquery, line 114 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could you add the case SELECT (SELECT 1, 2) IN (SELECT 1, 2);? It's another one that we support (and MySQL supports) but PG doesn't.

We already have that case on line 227.

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/subquery.go, line 404 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The comment was inaccurate. In a multi-row context (such as the right-hand side of an IN expression), we always wrap the inner type U in a tuple resulting in tuple{U}. If the subquery returns a single column, the type U is the type of that column. If the subquery returns multiple columns C, the type is tuple{C}. So for the above query, the type of the subquery will be tuple{int}.

I've rewritten the comment to make it clearer what the subquery typing rules ares.

I hope @knz will appreciate my the irony of my earlier comments about comments in light of the verbose comments present in this PR.

---

Comments from Reviewable

[knz]

for the code change -- even though you're looking into the tuple/vtuple distinction separately, could you expand the comment here to make a note this is a concern?

---

Reviewed 1 of 1 files at r1, 1 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/sql/subquery.go, line 404 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I hope @knz will appreciate my the irony of my earlier comments about comments in light of the verbose comments present in this PR.

I think there is wisdom in investing both in exhaustive, up-to-date comments and also learning how to read code effectively (and learning to verify code matches comments). I am working on it.

---

pkg/sql/subquery.go, line 411 at r3 (raw file):

	//
	//   - If the subquery returns multiple columns, the type of the subquery is
	//     "tuple{tuple{C}}" where "C expands to all of the types of the columns

aw the outer tuple{...} here should be vtuple, or at least since you're making a major improvement to this code you could also add a note here that reusing the base "tuple" type is technically incorrect because it requires fixed arity, and instead we'd like something with variable arity.

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/subquery.go, line 411 at r3 (raw file):

Previously, knz (kena) wrote…

aw the outer tuple{...} here should be vtuple, or at least since you're making a major improvement to this code you could also add a note here that reusing the base "tuple" type is technically incorrect because it requires fixed arity, and instead we'd like something with variable arity.

I've added a TODO about the misuse of the tuple type below where it gets added in the multi-row case.

---

Comments from Reviewable