-
Notifications
You must be signed in to change notification settings - Fork 139
fix: Avoid aliasing known CTEs #839
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: Avoid aliasing known CTEs #839
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Kokoro failure looks like #875 We might need to address that before we can merge. |
@@ -261,6 +261,7 @@ def _known_tables(self): | |||
if isinstance(from_, Table): | |||
known_tables.add(from_.name) | |||
elif isinstance(from_, CTE): | |||
known_tables.add(from_.name) |
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.
Any possibility you could add a test that was failing before this change? Or at least provide a code example so that one of the package maintainers could add such a test.
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.
https://github.com/googleapis/python-bigquery-sqlalchemy/blob/main/tests/system/test_sqlalchemy_bigquery.py looks like a pretty natural spot for such a test.
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.
Test added
I now also add the Common Table Expression (CTE) names to the known tables, so they will not be re-added (thus cross-joined with self).
The following snippet (from the issue) was used to validate the change:
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Related #368