-
Notifications
You must be signed in to change notification settings - Fork 272
fix: Support Trino bucketing syntax #252
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
Conversation
|
@chezou Thanks for contributing. I just approved the CI test. Could you fix the failed flake8 check first? |
|
@reata Thanks, I think I ran tox on my local env, but will rerun and fix them. |
Since Trino uses WITH for bucketing without Identifier after WITH, allow Parenthesis after WITH. https://trino.io/blog/2019/05/29/improved-hive-bucketing.html
|
@reata Fixed by confirming local tox and force pushed the commit. Can you approve to run GitHub workflow? |
Approved. Sorry I might be slow to respond recently because the covid-19 lockdown in Shanghai took a lot of energy from me. Please don't be discouraged. I really appreciate it. |
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
=======================================
Coverage 99.01% 99.02%
=======================================
Files 20 20
Lines 918 924 +6
=======================================
+ Hits 909 915 +6
Misses 9 9
Continue to review full report at Codecov.
|
reata
left a comment
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.
This PR looks good to me. It helps if you can answer some of my inline questions.
sqllineage/core/handlers/cte.py
Outdated
| elif isinstance(token, Parenthesis): | ||
| # CREATE TABLE tbl1 (col1 VARCHAR) WITH (bucketed_by = ARRAY['col1'], bucket_count = 256). | ||
| # This syntax is valid for bucketing in Trino and not the CTE | ||
| self._handle(token.tokens[1], holder) |
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 do we still want to handle token.tokens[1] here?
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.
Good catch. Fixed on 8ee1b34
I originally thought the parenthesis should be parsed, but looking at the examples, no longer needed.
reata
left a comment
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
Since Trino uses WITH for bucketing without Identifier after WITH,
allow Parenthesis after WITH.
https://trino.io/blog/2019/05/29/improved-hive-bucketing.html
Close #251