-
Notifications
You must be signed in to change notification settings - Fork 769
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
Feat(optimizer): add support for resolving CTEs in CREATE statements #1949
Feat(optimizer): add support for resolving CTEs in CREATE statements #1949
Conversation
@@ -1006,6 +1006,31 @@ class Create(Expression): | |||
"clone": False, | |||
} | |||
|
|||
@property | |||
def ctes(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we move these helper methods to a shared base class and have this applied to Insert and Update or other DDLs?
then in the optimizer you can check for the base class instead of just Create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call it DDL
thanks so much for the PR! |
@gtoonstra are you planning to finish up this pr? |
4ef83a1
to
c24acaf
Compare
@tobymao : I have made the necessary changes and CI is processing now. After this PR, I will look into INSERT/UPDATE as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left one question.
I agree with Toby about renaming TableChange
to DDL
. I believe INSERT
and UPDATE
should just work with a few changes (i.e. do instance checks on the common superclass instead of just CREATE
).
Btw, if we're to create a new test file for qualify columns, I'd rather use a bit more generic name like qualify_columns_ddl.sql
or something to anticipate for the INSERT
, UPDATE
tests as well.
Nice 👍
WITH cte AS (SELECT a FROM db.y) CREATE TABLE s AS SELECT * FROM cte; | ||
WITH cte AS (SELECT a AS a FROM db.y AS y) CREATE TABLE s AS SELECT cte.a AS a FROM cte; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't cte
have an alias in the final SELECT
here? Seems a bit off. Ditto for subsequent tests.
I'll complete this, but most of the work should be already done. Thanks for the PR @gtoonstra! |
Pull request to resolve CTEs when CREATE statement is at the root of the AST.
I've dived a lot more into the scopes and resolvers and came up with this implementation so far.
Looking forward to feedback.