optbuilder: Fix bugs with subqueries, DISTINCT, and ORDER BY

[rytaft]

This commit fixes several miscellaneous issues.

1. Subqueries with aggregation and without a FROM clause were
   causing the outer query to incorrectly act as if it were in
   a grouping context. This commit fixes the issue by making
   sure the scope of the subquery is different than the scope
   of the outer query.
2. Queries with DISTINCT and ORDER BY in which the ORDER BY
   was ordering on an expression that was also part of the
   SELECT list were failing. This commit fixes the issue by
   reusing the existing projection for the ORDER BY column.
3. Several redundant columns were previously being synthesized
   for identical expressions. This commit fixes the issue by
   reusing existing columns wherever possible.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 27 at r1 (raw file):

 │              0  ·       aggregate 9   bool_or(column16)   ·                                                                                                           ·
 │              0  ·       aggregate 10  xor_agg(column18)   ·                                                                                                           ·
 └── render     1  render  ·             ·                   (column5, column5, column5, column5, column5, column5, column5, column5, column14, column16, column18)      ·

This is odd - we are rendering the same column multiple times.. There should only be one render for column5.. Or are they different columns which have the same label for some reason?

Also, how come the corresponding testcase in optbuilder didn't change?

---

pkg/sql/opt/optbuilder/orderby.go, line 145 at r1 (raw file):

		// rebuilding the expression.
		if col := projectionsScope.findExistingCol(e); col != nil {
			orderByScope.cols[len(orderByScope.cols)-1] = *col

kind of confused why we modify the last column in the scope here? Is it the column added by buildScalarProjection? (could use a comment)

---

pkg/sql/opt/optbuilder/project.go, line 223 at r1 (raw file):

		outScope.cols = append(outScope.cols, *col)
		if label != "" {
			outScope.cols[len(outScope.cols)-1].name = tree.Name(label)

[nit] why not set this before appending? e.g. colCopy := *col; colCopy.name = label

---

Comments from Reviewable

[rytaft]

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

---

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 27 at r1 (raw file):

Previously, RaduBerinde wrote…

This is odd - we are rendering the same column multiple times.. There should only be one render for column5.. Or are they different columns which have the same label for some reason?

Also, how come the corresponding testcase in optbuilder didn't change?

The optbuilder test case did change - see the diff in optbuilder/testdata/aggregate

column5 is just the column created for constant 1. It's repeated in the pre-projection for each of the aggregates it's used in. But I agree it's unnecessary to repeat it - I'll try to consolidate it down to one projection.

---

pkg/sql/opt/optbuilder/orderby.go, line 145 at r1 (raw file):

Previously, RaduBerinde wrote…

kind of confused why we modify the last column in the scope here? Is it the column added by buildScalarProjection? (could use a comment)

Yep it's added by buildScalarProjection I'll add a comment

---

pkg/sql/opt/optbuilder/project.go, line 223 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] why not set this before appending? e.g. colCopy := *col; colCopy.name = label

I wanted to avoid copying twice... but I can change it if you think it would be more readable.

---

Comments from Reviewable

[RaduBerinde]

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

---

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 27 at r1 (raw file):

Previously, rytaft wrote…

The optbuilder test case did change - see the diff in optbuilder/testdata/aggregate

column5 is just the column created for constant 1. It's repeated in the pre-projection for each of the aggregates it's used in. But I agree it's unnecessary to repeat it - I'll try to consolidate it down to one projection.

Ah, indeed, reviewboard hid those diffs from me haha

It seems semantically dubious to have a project that creates the same column multiple times (it only makes any sense if the renderings are identical).

---

pkg/sql/opt/optbuilder/project.go, line 223 at r1 (raw file):

Previously, rytaft wrote…

I wanted to avoid copying twice... but I can change it if you think it would be more readable.

Maybe just hide it inside a helper function outScope.AppendCol(col, label)

---

Comments from Reviewable

[rytaft]

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

---

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 27 at r1 (raw file):

Previously, RaduBerinde wrote…

Ah, indeed, reviewboard hid those diffs from me haha

It seems semantically dubious to have a project that creates the same column multiple times (it only makes any sense if the renderings are identical).

Removed all the extra projections!

---

pkg/sql/opt/optbuilder/orderby.go, line 145 at r1 (raw file):

Previously, rytaft wrote…

Yep it's added by buildScalarProjection I'll add a comment

Done.

---

pkg/sql/opt/optbuilder/project.go, line 223 at r1 (raw file):

Previously, RaduBerinde wrote…

Maybe just hide it inside a helper function outScope.AppendCol(col, label)

Done.

---

Comments from Reviewable

[RaduBerinde]

---

Review status: 0 of 16 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 27 at r1 (raw file):

Previously, rytaft wrote…

Removed all the extra projections!

👍

---

pkg/sql/opt/optbuilder/project.go, line 157 at r2 (raw file):

	}
	out := b.factory.ConstructVariable(b.factory.InternColumnID(col.id))
	outScope.appendColumn(col, label)

[nit] appendColumn could return &outScope.cols[len(outScope.cols)-1]

---

pkg/sql/opt/optbuilder/util.go, line 136 at r2 (raw file):

) memo.GroupID {
	colList := make(opt.ColList, 0, len(cols))
	itemList := make([]memo.GroupID, 0, len(items))

[nit] this suggests cols and items can have different lengths, I'd just use len(cols). The loop below can never add more than that anyway

---

Comments from Reviewable

[rytaft]

TFTR! Updated.

---

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

---

pkg/sql/opt/optbuilder/project.go, line 157 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] appendColumn could return &outScope.cols[len(outScope.cols)-1]

Done.

---

pkg/sql/opt/optbuilder/util.go, line 136 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] this suggests cols and items can have different lengths, I'd just use len(cols). The loop below can never add more than that anyway

Done.

---

Comments from Reviewable

[RaduBerinde]

Reviewed 2 of 10 files at r1, 11 of 14 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[rytaft]

bors r=RaduBerinde

---

Review status: 0 of 16 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[craig]

Build succeeded

• GitHub CI (Cockroach)