This change is 

This is looking really good! I added a few remarks. Also, for the optimizer stuff, it would be good to get someone from the optimizer team to take a look.

Reviewed 1 of 1 files at r1, 17 of 17 files at r2, 7 of 7 files at r3.
Review status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_decompose.go, line 1519 at r2 (raw file):

}

func mergeSorted(

Please tell me there is a unit test for this. Cause it surely needs one badly.

I'm not a fan of the slicing up of the typs here. Why not just keep two index variables around instead of this fanciness?

pkg/sql/opt_decompose.go, line 1577 at r2 (raw file):

}

func intersectSorted(

This too, needs a unit test too.

pkg/sql/logictest/testdata/logic_test/srfs, line 782 at r3 (raw file):

statement ok
CREATE TABLE j(x INT PRIMARY KEY, y JSON);

Is this needed?

pkg/sql/logictest/testdata/logic_test/srfs, line 795 at r3 (raw file):

2  222

query I colnames

Can you add subtests for these new tests?

pkg/sql/logictest/testdata/logic_test/srfs, line 838 at r3 (raw file):

    JOIN pg_catalog.pg_namespace n ON (ct.relnamespace = n.oid)
    JOIN (SELECT i.indexrelid,
		 i.indrelid,

Leftover tabs

pkg/sql/logictest/testdata/logic_test/srfs, line 895 at r3 (raw file):

# WHERE TRUE
#   AND n.nspname = 'public'
#   AND ct.relname = 'j'

Was this change intentional?

pkg/sql/logictest/testdata/logic_test/tuple, line 763 at r3 (raw file):

1

statement ok

Move this declaration of t down to where it is used.

pkg/sql/logictest/testdata/logic_test/tuple, line 768 at r3 (raw file):

statement ok
INSERT INTO t VALUES (1, 'one'), (2, 'two')

extra empty line

pkg/sql/opt/exec/execbuilder/scalar_builder.go, line 125 at r2 (raw file):

		}
	}
	typ := ev.Logical().Scalar.Type.(types.TTuple)

Move this into the return statement directly?

pkg/sql/opt/memo/extract.go, line 48 at r2 (raw file):

			datums[i] = ExtractConstDatum(ev.Child(i))
		}
		typ := ev.Logical().Scalar.Type.(types.TTuple)

This one can go in the return statement too.

pkg/sql/sem/tree/datum.go, line 2484 at r2 (raw file):

	// The Labels sub-field can be left nil. If populated, it must have
	// the same arity as D.
	typ types.TTuple

Should this be a pointer? Is this not adding a ttuple to every dtuple?

Also, why call this typ? How about ttuple so there is no ambiguity?

pkg/sql/sem/tree/eval.go, line 3486 at r1 (raw file):

// evalArgs evaluates just the function application's arguments.
// A 1st result bool true indicates NULL should be propagated.

This is worded awkwardly.

The returned bool indicates that the NULL should be propagated.

pkg/sql/sem/tree/expr.go, line 1509 at r3 (raw file):

	Expr     Expr
	ColName  string
	Star     bool

Add a note that Star may be mutated during type check.

pkg/sql/sem/tree/type_check_test.go, line 257 at r3 (raw file):

			`star expansion of tupled non-relational scalars is not supported`,
		},
		// TODO(bram): same label failure case here ********

Probably a good idea to do as my leftover todo says. :)

Comments from Reviewable

Review status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt_decompose.go, line 1519 at r2 (raw file):

The tests are in opt_decompose_test.go.

But I reverted these changes. They are not needed because labels in IN comparisons never matter.

pkg/sql/opt_decompose.go, line 1577 at r2 (raw file):

ditto

pkg/sql/opt/exec/execbuilder/scalar_builder.go, line 125 at r2 (raw file):

I find it harder to read.

pkg/sql/opt/memo/extract.go, line 48 at r2 (raw file):

ditto

pkg/sql/sem/tree/datum.go, line 2484 at r2 (raw file):

The name typ repeats the name used throughout type_check.go (typeAnnotation).

The use by value instead of pointer is because of #26680. tree.Tuple does the same. Added a TODO.

pkg/sql/sem/tree/eval.go, line 3486 at r1 (raw file):

Done.

Comments from Reviewable

pkg/sql/opt_decompose.go, line 1519 at r2 (raw file):

Ah, cool. But if you did keep it in, you needed to augment those tests with labels.

Comments from Reviewable

pkg/sql/opt/exec/execbuilder/scalar_builder.go, line 125 at r2 (raw file):

ok

Comments from Reviewable

Reviewed 22 of 22 files at r4, 17 of 17 files at r5, 7 of 7 files at r6.
Review status: complete! 0 of 0 LGTMs obtained

Comments from Reviewable

opt stuff LGTM

Reviewed 7 of 17 files at r5.
Review status: complete! 0 of 0 LGTMs obtained

Comments from Reviewable

Review status: complete! 0 of 0 LGTMs obtained

pkg/sql/logictest/testdata/logic_test/srfs, line 782 at r3 (raw file):

What does the word "this" refer to in your question?

pkg/sql/logictest/testdata/logic_test/srfs, line 795 at r3 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/srfs, line 838 at r3 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/srfs, line 895 at r3 (raw file):

Yes because there is no index in the virtual table information_schema.columns.

pkg/sql/logictest/testdata/logic_test/tuple, line 763 at r3 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/tuple, line 768 at r3 (raw file):

Done.

pkg/sql/sem/tree/expr.go, line 1509 at r3 (raw file):

Changed to a separate type TupleStar.

pkg/sql/sem/tree/type_check_test.go, line 257 at r3 (raw file):

Can you detail what you mean?

pkg/sql/opt_decompose.go, line 1519 at r2 (raw file):

I agree.

Comments from Reviewable

Review status: complete! 0 of 0 LGTMs obtained

pkg/sql/sem/tree/type_check_test.go, line 257 at r3 (raw file):

Removed comment as per offline discussion.

Comments from Reviewable

Probably should have opt team do one last pass for the new changes.

Reviewed 20 of 20 files at r7, 4 of 4 files at r8.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/targets.go, line 70 at r7 (raw file):

	}

	if hasStar, cols, typedExprs, err := sqlbase.CheckRenderStar(ctx, p.analyzeExpr,

This is even worse than going over the line limit. How about

if hasStar, cols, typedExprs, err := sqlbase.CheckRenderStar(
  ctx, p.analyzeExpr, target, info, ivarHelper,
); err != nil {
...

pkg/sql/logictest/testdata/logic_test/srfs, line 782 at r3 (raw file):

I think it was for table j. Looks like it is needed. So just ignore this. Probably the code was hidden by reviewable when reviewing.

Comments from Reviewable

@rytaft: I have extended/reworked the opt star expansion. PTAL.

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/targets.go, line 70 at r7 (raw file):

Good idea. Done.

Comments from Reviewable

Reviewed 3 of 3 files at r9.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

Comments from Reviewable

Reviewed 6 of 20 files at r7, 1 of 3 files at r9.
Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

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

		}

		// Special handling for "*", "<table>.*" and "(E).*".

I'd change this to (Expr).* or (Tuple).* (or something along those lines) to avoid confusion

pkg/sql/opt/optbuilder/scope.go, line 533 at r9 (raw file):

	switch t := expr.(type) {
	case *tree.TupleStar:
		// TupleStar at the top level of a SELECT clause are replaced

TupleStar -> TupleStars (or change are -> is)

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

// expandStar expands expr into a list of columns if expr
// corresponds to a "*" or "<table>.*".

Update comment to include TupleStar.

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

		// If the sub-expression is a tuple constructor, we'll de-tuplify below.
		// Otherwise we'll re-evaluate the expression multiple times.
		tTuple, isTuple := texpr.(*tree.Tuple)

When is the type TTuple but the expression is not actually a tuple? An example would help.

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

// expandStarAndResolveType expands expr into a list of columns if expr
// corresponds to a "*" or "<table>.*". Otherwise, expandStarAndResolveType

Update comment to include TupleStar

Comments from Reviewable

Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

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

Done.

pkg/sql/opt/optbuilder/scope.go, line 533 at r9 (raw file):

Done.

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

Done.

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

Added example.

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

Done.

Comments from Reviewable

thanks for the reviews!

bors r+

Build failed

• GitHub CI (Cockroach)

Had to fix #26720 too. Retrying.

bors r+

Build succeeded

• GitHub CI (Cockroach)