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.

Sign up for GitHub

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

Jump to bottom

Alias resolution updates #1281

Merged
merged 29 commits into from
Sep 27, 2022
Merged

Alias resolution updates #1281

merged 29 commits into from
Sep 27, 2022

Conversation

fulghum
Copy link
Contributor

@fulghum fulghum commented Sep 23, 2022 •
edited

This change moves our alias identification and column name qualification code closer to MySQL's behavior. The major changes are:

  • identify available alias, table, and column names on a per-scope level (instead of the previous "nestingLevel" that was based off of the node depth of the execution plan tree)
  • supply additional context to the qualifyExpression method so that the containing node can be used to switch on different alias visibility rules

There are still more nuanced edge cases in MySQL's alias behavior that we should continue working on matching (e.g. dolthub/dolt#4535, dolthub/dolt#4537), but this change moves us a nice step forward.

Dolt CI Tests:

Fixes:

@fulghum
Switching column alias test definitions to a ScriptTest (instead of Q…
19afe3f 
…ueryTests)
@fulghum
Adding more tests for column aliases
9403a6e
@fulghum
Enabling ScriptTestAssertions to test expected columns
9893e2b
@fulghum
Adding expression.AliasReference to record when we have identified an…
7435a7b 
… expression as an alias
@fulghum
Updating test queries that filter on aliases to use having instead of…
33023ad 
… where, since SQL standard prevents aliases in where clause
@fulghum
Minor tweaks/comments
edd2446
@fulghum
Qualifying common columns as part of resolveNaturalJoins rule to prev…
4bec07a 
…ent ambiguity later in qualifyColumns.
@fulghum
Switching from Filter to Having for a few SHOW statements that rely o…
80a2f26 
…n aliases (which aren't allowed to be used in where clauses)
@fulghum
First pass of changes for resolve_columns
c9b836c
@fulghum
Refining expression alias test queries
139282f
@fulghum
Updates to reorder_projections for new alias resolution logic
b9c9683
@fulghum
Various changes for new alias resolution and start of renaming "nesti…
27946bd 
…ngLevel" to "scopeLevel"
@fulghum
Various changes for new alias resolution to select between match tabl…
d66057c 
…es and alias names
@fulghum
Giving ScriptTestAssertion the ability to specify an assertion as ski…
f58c404 
…pped and temporarily skipping two failing column alias tests.
@fulghum
Fixing some unit tests
eb65be3
@fulghum
Keeping the GetField expression instead of creating an UnresolvedColu…
640dc9b 
…mn expression
@fulghum
Updating comments
a6e3696
@fulghum
Renaming for clarity with scoping
dd578e2
@fulghum
More renaming, commenting, and cleaning up
58c3d67
@fulghum
Renaming findDeferredColumns now that it searches for AliasReferences…
632b66c 
…, too
@fulghum
Remove old, out of date comment
0123d74
@fulghum
Adding a comment about MySQL's support for outer scope references for…
fcfebb8 
… derived tables
@fulghum
Cleaning up todos, adding comments, and introducing new ErrAmbiguousC…
1b0deef 
…olumnOrAliasName error kind.
@fulghum
Cleaning up logic in resolve_columns for identifying aliases and qual…
86c9aab 
…ifying column references.
@fulghum
Merge branch 'main' into fulghum/analyzer-aliases
e0a2ec0
@fulghum fulghum changed the title WIP: Alias resolution updates \Alias resolution updates Sep 26, 2022
@fulghum fulghum changed the title \Alias resolution updates Alias resolution updates Sep 26, 2022
@fulghum fulghum marked this pull request as ready for review September 26, 2022 22:42
@fulghum fulghum linked an issue Sep 27, 2022 that may be closed by this pull request
Column alias in where clause should be an error #525
Closed
@fulghum fulghum mentioned this pull request Sep 27, 2022
Closed
max-hoffman
max-hoffman approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM, only a couple small comments. Thanks for cleaning resolveColumns up, it has been getting increasingly fragile.

sql/analyzer/resolve_columns.go Outdated Show resolved Hide resolved
sql/analyzer/resolve_columns.go Show resolved Hide resolved
sql/analyzer/resolve_columns.go Show resolved Hide resolved
zachmu
zachmu approved these changes Sep 27, 2022
View reviewed changes
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, not perfect but steps in the right direction

sql/analyzer/resolve_columns.go Outdated Show resolved Hide resolved
sql/analyzer/resolve_columns.go Show resolved Hide resolved
sql/analyzer/resolve_columns.go Outdated Show resolved Hide resolved
@fulghum fulghum merged commit cff88fe into main Sep 27, 2022
@Hydrocharged Hydrocharged deleted the fulghum/analyzer-aliases branch October 13, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachmu zachmu zachmu approved these changes

@max-hoffman max-hoffman max-hoffman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Column alias in where clause should be an error
3 participants
@fulghum @zachmu @max-hoffman