Reviewed 14 of 15 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

pkg/sql/opt/optbuilder/builder.go line 148 at r2 (raw file):

	// If set, the data source names in the AST are rewritten to the table ID and
	// all the visible column IDs.
	useIDsInAST bool

[nit] Could this be replaced by insideFuncDef?

pkg/sql/opt/optbuilder/select.go line 112 at r2 (raw file):

		ds, depName, resName := b.resolveDataSource(tn, privilege.SELECT)
		if b.useIDsInAST {

Why replace the data source instead of the *?

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

) (aliases []string, exprs []tree.TypedExpr) {
	if b.insideViewDef {
		panic(unimplemented.NewWithIssue(10028, "views do not currently support * expressions"))

Can we also support * in views the same way?

pkg/sql/logictest/testdata/logic_test/udf line 127 at r2 (raw file):

  LANGUAGE SQL
  AS $$
  SELECT a FROM [113(1, 2, 3) AS t];

For outputs like this one, are we sure we want to show the rewritten version instead of the original?

pkg/sql/logictest/testdata/logic_test/udf_star line 95 at r2 (raw file):

statement ok
ALTER TABLE t_twocol ADD COLUMN c INT DEFAULT 5;

What happens if you change the column type (both to something compatible and something not compatible)? Or the column name?

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

pkg/sql/logictest/testdata/logic_test/udf_star line 95 at r2 (raw file):

Or the column name?

Oops, didn't notice you had this case already.

Also, could you add one where f_allcolsel is called after the table is altered? Postgres actually seems ok with that case:

postgres=# create table t (a int);
CREATE TABLE
postgres=# create function f() returns t language sql as 'select * from t';
CREATE FUNCTION
postgres=# alter table t add column b int;
ALTER TABLE
postgres=# select f();
 f
---

(1 row)

postgres=# insert into t values(1);
INSERT 0 1
postgres=# select f();
  f
------
 (1,)
(1 row)

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

pkg/sql/logictest/testdata/logic_test/udf_star line 95 at r2 (raw file):

Postgres actually seems ok with that case

Nevermind, I didn't realize we wanted to default to early binding.

TFTR!

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

pkg/sql/opt/optbuilder/builder.go line 148 at r2 (raw file):

Done.

pkg/sql/opt/optbuilder/select.go line 112 at r2 (raw file):

Using table and column IDs should make handling schema changes like column and table renaming easier, since the underlying IDs don't change. Otherwise when we rename something we'll need to rewrite the UDF body, I think.

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

Maybe, thanks for pointing that out. Let me look into that after this PR goes out. It looks like there's a long history about views and schema changes.

pkg/sql/logictest/testdata/logic_test/udf line 127 at r2 (raw file):

I discussed this with @mgartner offline, and we decided that ultimately we want to support both pretty printing with the table names and printing the rewritten query (like postgres does with \df+ and pg_catalog.pg_proc's prosqlbody, respectively). But now that I'm looking at this again, we are using pg_catalog.pg_proc here, though we seem to be using prosrc and prosqlbody differently than postgres given our different binding treatment for the same syntax... I will solicit opinions in the #udf channel about where we expect pretty versions of the udf vs not.

@mgartner is probably right that we need to reconstruct a pretty version of the body as opposed to the original source. This is what postgres appears to do for early binding UDFs, since altering the schema results in changes to what is output by \df+.

pkg/sql/logictest/testdata/logic_test/udf_star line 95 at r2 (raw file):

My understanding is that we only support early binding right now. I added a comment to clarify that this behavior is only expected for early binding. Added more ALTER examples in the test, which work as expected.

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

pkg/sql/logictest/testdata/logic_test/udf line 127 at r2 (raw file):

Actually, using the rewritten version here may break backups. Might need to reinterpret the udf body here.

I think this sans the possible backport issue.

Reviewed 1 of 1 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @mgartner, and @rharding6373)

pkg/sql/opt/optbuilder/select.go line 112 at r2 (raw file):

Makes sense, it looks like it would be very invasive to make that change on second glance.

pkg/sql/opt/optbuilder/select.go line 46 at r3 (raw file):

// See Builder.buildStmt for a description of the remaining input and
// return values.
func (b *Builder) buildDataSource(

[nit] I think we should either expand the comment here to mention that it can mutate texpr, or maybe change its contract to return the replacement. Do you know if we do this sort of thing anywhere else in the optbuilder?

pkg/sql/logictest/testdata/logic_test/udf_star line 105 at r3 (raw file):

# ok for late binding.
query T
SELECT f_unqualified_twocol()

I wonder if we should consider this a valid departure from postgres like defaulting to early binding - it seems reasonable to also early-bind the return type. Do you know what happens if a UDT is used as the return type and later altered?

Nice work on this! I learned a lot here. But as we chatted through slack that we'd prefer expanding * into column names instead of rewriting the data source. Not sure how much more work that would be to do the expansion, but it would save a lot of work on deserialization for things like SHOW CREATE FUNCTION.

I reimplemented to modify the AST in place to do * expansion instead of datasource replacement. RFAL, thanks!

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

pkg/sql/opt/optbuilder/select.go line 112 at r2 (raw file):

Reimplemented to expand the *, PTAL

pkg/sql/opt/optbuilder/select.go line 46 at r3 (raw file):

We do some minor tweaks to the AST, such as fully qualifying table names, but afaict we don't replace expressions often. I've added a comment to analyzeSelectList, which is where the expansion happens in the reimplementation.

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

With the reimplementation it's not really relevant anymore, but the work slated to #83233 is more closely aligned with that goal, it seems.

pkg/sql/logictest/testdata/logic_test/udf line 127 at r2 (raw file):

No longer relevant with the rewrite

pkg/sql/logictest/testdata/logic_test/udf_star line 105 at r3 (raw file):

Renaming the return type is fine, but adding attributes results in a return type mismatch error in postgres:

rharding=# create type typ as (a int);
CREATE TYPE
rharding=# create function f() returns typ language sql begin atomic; select 1; end;
CREATE FUNCTION
rharding=# select f();
  f  
-----
 (1)
(1 row)
rharding=# alter type typ rename to rtyp;
ALTER TYPE
rharding=# select f();
  f  
-----
 (1)
(1 row)
rharding=# alter type rtyp add attribute b int;
ALTER TYPE
rharding=# select f();
ERROR:  return type mismatch in function declared to return rtyp
DETAIL:  Final statement returns too few columns.
CONTEXT:  SQL function "f" during inlining

Nice work!

Reviewed 2 of 15 files at r1, 23 of 23 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @dt, and @rharding6373)

pkg/sql/opt/optbuilder/project.go line 153 at r4 (raw file):

						expanded = true
						for _, expr := range exprs {
							col := expr.(*scopeColumn)

I don't think the exprs are guaranteed to be *scopeColumns:

Is there a way to exercise that case in a UDF?

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

CREATE FUNCTION f_subquery() RETURNS INT AS
$$
  SELECT * FROM (SELECT a FROM (SELECT * FROM t_onecol) AS foo) AS bar;

Can you add a test like this without the subquery aliases foo and bar? If we can't support it because we the column names can't be fully qualified, then we can return a user error if there are unaliased subqueries in a UDF.

pkg/sql/opt/optbuilder/testdata/udf line 1181 at r4 (raw file):

exec-ddl
CREATE FUNCTION fn_star() RETURNS INT LANGUAGE SQL AS 'SELECT * FROM tstar'

Might be nice to have another, more complicated test case here, like with a subquery where the column qualification is important. It'll make sure that testcat respects the rewriting correctly, too.

pkg/ccl/backupccl/backup_test.go line 10630 at r4 (raw file):

$$;
`)

nit: revert this change

Reviewed 23 of 23 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @mgartner, and @rharding6373)

pkg/sql/opt/optbuilder/project.go line 129 at r4 (raw file):

) {
	var expansions tree.SelectExprs
	expanded := false

[nit] Since we're filling out expansions unconditionally when b.insideFuncDef, checking expanded doesn't seem useful.

pkg/sql/opt/optbuilder/select.go line 47 at r4 (raw file):

// return values.
func (b *Builder) buildDataSource(
	texpr *tree.TableExpr, indexFlags *tree.IndexFlags, locking lockingSpec, inScope *scope,

[nit] Is the change here still necessary?

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @DrewKimball, @dt, and @mgartner)

pkg/sql/opt/optbuilder/project.go line 129 at r4 (raw file):

Done.

pkg/sql/opt/optbuilder/project.go line 153 at r4 (raw file):

Good catch. Fixed this and added a test case.

pkg/sql/opt/optbuilder/select.go line 47 at r4 (raw file):

Done. Thanks for the catch!

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

Doesn't seem like we do this correctly outside of UDFs, but maybe this is intentional. Thoughts?

In postgres:

rharding=# create table t (a int);
CREATE TABLE
rharding=# select * from (select a from (select * from t));
ERROR:  subquery in FROM must have an alias
LINE 1: select * from (select a from (select * from t));
                                     ^
HINT:  For example, FROM (SELECT ...) [AS] foo.

In CRDB we don't error:

root@127.0.0.1:26257/defaultdb> create table t (a int);                                               
CREATE TABLE

root@127.0.0.1:26257/defaultdb> select * from (select a from (select * from t));                      
  a
-----
(0 rows)
root@127.0.0.1:26257/defaultdb> explain (opt, verbose) select * from (select a from (select * from t));                                                                  
             info
-------------------------------
  scan t
   ├── columns: a:1
   ├── stats: [rows=1]
   ├── cost: 25.68
   ├── distribution: us-east1
   └── prune: (1)
(6 rows)

pkg/sql/logictest/testdata/logic_test/udf_star line 61 at r4 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/udf_star line 140 at r4 (raw file):

Done. Let me know if you think we need to test more cases than dropping columns and tables, in which case it might make more sense to expand the unit test to include a function with select *.

pkg/sql/opt/optbuilder/testdata/create_function line 45 at r4 (raw file):

Yes, done!

pkg/sql/opt/optbuilder/testdata/udf line 1181 at r4 (raw file):

Done.

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

Reviewed 5 of 10 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @DrewKimball, @dt, and @rharding6373)

pkg/sql/opt/optbuilder/project.go line 158 at r6 (raw file):

							case *tree.ColumnAccessExpr:
								expansions = append(expansions, tree.SelectExpr{Expr: col})
							}

I think we should add a default case that panics with an assertion failed error rather than continuing on successfully.

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

Right, CRDB has allowed unaliased subqueries for some time. It's probably a relic from some early implementation rather than a specific decision we made to allow it. Since we allow it, we need to figure out what to do when they are present in UDFs with star expansions. If you add a test with a UDF body of: SELECT * FROM (SELECT a FROM (SELECT * FROM t_onecol));, what do the stars get expanded to?

pkg/sql/logictest/testdata/logic_test/udf_star line 79 at r6 (raw file):

100115  f_unqualified_doublestar  SELECT t_onecol.a, t_onecol.a FROM test.public.t_onecol;
                                  SELECT a FROM test.public.t_onecol;
100116  f_exprstar                SELECT word FROM (SELECT (pg_get_keywords()).word, (pg_get_keywords()).catcode, (pg_get_keywords()).catdesc ORDER BY word LIMIT 1);

This rewrite is a bit unfortunate - now we're evaluating the pg_get_keywords() function multiple times. I wonder if there is a more efficient rewrite we can do.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @DrewKimball, @dt, and @mgartner)

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

The expansion (in the current implementation) is SELECT a FROM (SELECT a FROM (SELECT t_onecol.a FROM test.public.t_onecol))

If we allow it outside of UDFs, wouldn't it make sense to allow it inside UDFs even if it means they both differ in comparison to postgres? Why do we prefer different behavior in UDFs than outside of UDFs?

pkg/sql/logictest/testdata/logic_test/udf_star line 79 at r6 (raw file):

Not very efficient, but happens to be what postgres does in its rewrite so I decided to leave it for now.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @DrewKimball, @dt, and @rharding6373)

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

I think we should allow it, if possible, but I'm worried that without an alias for the subquery, this rewrite will produce a query that's either ambiguous or not equivalent to the original. I can see my last example wasn't interesting, so let me try again. Consider the query:

CREATE TABLE t1 (a INT);
CREATE TABLE t2 (a INT);

SELECT * FROM (SELECT a FROM t1) JOIN (SELECT a FROM t2) ON true;

This should produce two columns, one from each side of the join. Without an alias for the subqueries, I'm assuming we'd rewrite this as:

SELECT a, a FROM (SELECT a FROM t1) JOIN (SELECT a FROM t2) ON true;

Both as are ambiguous, and if we try to run this we get the error: column reference "a" is ambiguous.

pkg/sql/logictest/testdata/logic_test/udf_star line 79 at r6 (raw file):

👍

Thanks for all the careful reviews!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @DrewKimball, @dt, and @mgartner)

pkg/sql/opt/optbuilder/project.go line 158 at r6 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

I opened an issue to address ambiguous columns in subqueries here: #96375

We now throw an unimplemented error for unaliased subqueries, could you take a look at optbuilder/select.go? TY!

Nice work!

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @DrewKimball, and @dt)

pkg/sql/logictest/testdata/logic_test/udf_star line 18 at r4 (raw file):

👍