Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve IN_SUBQUERY table disambiguation #2181

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Dec 4, 2023

When unnesting and IN_SUBQUERY into a parent scope with a table name clash, rename the child table and update its references to the new name. Prevent EXISTS subqueries from unnesting if it doesn't full decorrelate the child scope.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// renameExpressionTables renames table references recursively. We use
// table ids to avoid improperly renaming tables in lower scopes with the
// same name.
func renameExpressionTables(n sql.Node, rename map[sql.TableId]string) (sql.Node, transform.TreeIdentity, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine but I'm a little confused why this should be necessary. Isn't the point of table IDs so that we can uniquely identify every table without doing string matching?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assignExecIndexes still does string matching right now, but in general it would be nice to expand unique id use

duplicates := make(map[string]int)
n, _, err := transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) {
switch n := n.(type) {
func disambiguateTables(used map[string]int, n sql.Node) (sql.Node, map[string]int, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the return params

@max-hoffman max-hoffman merged commit e27c229 into main Dec 5, 2023
7 checks passed
@max-hoffman max-hoffman deleted the max/in-subuery-table-names branch December 5, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants