opt/optbuilder: support using table name as projection #64748
Conversation
|
This change is |
|
I'm not sure this is the best strategy for dealing with it, but other stuff i've tried (e.g. try make tree_name_resolution recognise these) doesn't work out :\ |
otan
force-pushed
the
expand_star
branch
3 times, most recently
from
a2eda62 to
2213c7e
Compare
There was a problem hiding this comment.
This name resolution stuff is a headache... thanks for working on this!
Does this also fix #45854?
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan and @RaduBerinde)
pkg/sql/opt/optbuilder/project.go, line 200 at r1 (raw file):
panic(resolveErr) } }()
Completely ignoring the source of the panic here makes me nervous. We already have an issue open to audit the panic catching code in the optimizer since it may be catching panics that it shouldn't.
It would be great to at least put this in a helper function (e.g., tryRunFunction) to limit the proliferation of new panic catchers, but I wonder if it's possible to avoid the need for this completely.
For example, could you call some other function to check whether colName is actually a table name or something that can be resolved to a tuple?
pkg/sql/opt/optbuilder/scope.go, line 946 at r1 (raw file):
Parts: tree.NameParts{"", string(t.ColumnName)}, }, }
I think you could put this into a little helper function makeTupleStarFromTableName or something like that (and reuse in project.go).
pkg/sql/opt/optbuilder/testdata/select, line 1508 at r1 (raw file):
exec-ddl CREATE TABLE same_name (
Does the behavior you're testing here match Postgres?
There was a problem hiding this comment.
Does this also fix #45854?
yep! added a test
Reviewable status:
pkg/sql/opt/optbuilder/project.go, line 200 at r1 (raw file):
For example, could you call some other function to check whether colName is actually a table name or something that can be resolved to a tuple?
not sure where to pull that data from :(. resolveType seems to do it.
i think the panic catch here is safe because we're always falling back to the "original" panic. it is already wrapped in a defer func, i cannot re-use this as it has a different return type to the other.
pkg/sql/opt/optbuilder/scope.go, line 946 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I think you could put this into a little helper function
makeTupleStarFromTableNameor something like that (and reuse in project.go).
Done.
pkg/sql/opt/optbuilder/testdata/select, line 1508 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Does the behavior you're testing here match Postgres?
yep!
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/opt/optbuilder/project.go, line 200 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
For example, could you call some other function to check whether colName is actually a table name or something that can be resolved to a tuple?
not sure where to pull that data from :(. resolveType seems to do it.
i think the panic catch here is safe because we're always falling back to the "original" panic. it is already wrapped in a defer func, i cannot re-use this as it has a different return type to the other.
I don't think it's necessarily safe, because the original panic may have been ok (e.g., originating from the optimizer code), while the new panic might have actually put the DB into a bad state (e.g., some type resolution code querying the DB causes a panic at the kv layer). We're not differentiating between these two cases today, but that is something I want to fix soon.
Let me poke around to see if I can suggest a better function for you to call...
* Fix projection code to be able to use the column name as a table name by catching undefined columns error and evaluating it as a TupleStar * Fix scope VisitPre to do something similar to above, to make this work for functions. Release note (sql change): Using the table name as a projection now works, e.g. SELECT table_name FROM table_name or SELECT row_to_json(table_name) FROM table_name.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/project.go, line 200 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I don't think it's necessarily safe, because the original panic may have been ok (e.g., originating from the optimizer code), while the new panic might have actually put the DB into a bad state (e.g., some type resolution code querying the DB causes a panic at the kv layer). We're not differentiating between these two cases today, but that is something I want to fix soon.
Let me poke around to see if I can suggest a better function for you to call...
i've changed it to check for UndefinedTable, in which case I'll panic with the original error.
Otherwise, it panics with the new error.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:
pkg/sql/opt/optbuilder/project.go, line 200 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
i've changed it to check for UndefinedTable, in which case I'll panic with the original error.
Otherwise, it panics with the new error.
Thanks! Sorry I didn't get back to this before you did!
|
no worries, tftr! bors r=rytaft |
|
Build succeeded: |
by catching undefined columns error and evaluating it as a TupleStar
for functions.
Resolves #60549
Resolves #26719
Resolves #45854
Release note (sql change): Using the table name as a projection now
works, e.g. SELECT table_name FROM table_name or SELECT
row_to_json(table_name) FROM table_name.