-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
fix: ParsedQuery subselect edge case #13602
fix: ParsedQuery subselect edge case #13602
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13602 +/- ##
==========================================
- Coverage 80.93% 70.96% -9.98%
==========================================
Files 304 828 +524
Lines 24807 41443 +16636
Branches 0 4300 +4300
==========================================
+ Hits 20077 29409 +9332
- Misses 4730 12034 +7304
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
I thiiink this should be safe. Can you check to make sure other uses of parentheses like SELECT f1, (x + y) AS f2 FROM t1
work as expected?
superset/sql_parse.py
Outdated
@@ -278,7 +285,11 @@ def _extract_from_token( # pylint: disable=too-many-branches | |||
table_name_preceding_token = False | |||
|
|||
for item in token.tokens: | |||
if item.is_group and not self._is_identifier(item): | |||
if (item.is_group and not self._is_identifier(item)) or ( |
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.
item.is_group and (not self.is_identifier(item) or isinstance(item.tokens[0], Parenthesis))
?
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.
i think it's equivalent, do you think it's as readable?
62d304b
to
4cc8dc5
Compare
add the parens test you recommended |
SUMMARY
I found that aliased subselects would break the logic to extract table names from a query. When using sqlparse, it seemed like the query got expanded more than expected when the alias was added:
This adds logic to recursively explore sections wrapped in parens, which seems safe and didn't break any of the existing tests while allowing mine to pass. That said, would love any thoughts on a better way to do this too
TEST PLAN
CI
to: @villebro @lilykuang @serenajiang @bkyryliuk @michellethomas