-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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(sql lab): Use quote_schema instead of quote method to format schema name #26281
fix(sql lab): Use quote_schema instead of quote method to format schema name #26281
Conversation
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26281 +/- ##
=======================================
Coverage 69.18% 69.18%
=======================================
Files 1945 1945
Lines 75948 75950 +2
Branches 8458 8458
=======================================
+ Hits 52546 52548 +2
Misses 21217 21217
Partials 2185 2185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice! A quick glance at my venv
revealed that at least the default MSSQL and Snowflake dialects are overriding this with their own logic, with the default falling back to quote
as stated here. LGTM
@@ -1433,8 +1433,9 @@ def select_star( # pylint: disable=too-many-arguments,too-many-locals | |||
if show_cols: | |||
fields = cls._get_fields(cols) | |||
quote = engine.dialect.identifier_preparer.quote | |||
quote_schema = engine.dialect.identifier_preparer.quote_schema |
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.
Daydreaming: How I wish Python had this syntax like JS/TS does..
{ quote, quote_schema } = engine.dialect.identifier_preparer
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.
TIL I learned that quote_schema
was a thing. Do we need to use this everywhere we use the quote
method?
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.
Wherever a schema is the input argument, yes. For table names and the like, quote
is fine. As far as I could tell this is the only place where a schema name is formatted
Please merge this PR. We need it to address this issue: #26286 |
…ma name (apache#26281) (cherry picked from commit 9d37968)
SUMMARY
DuckDB supports multiple databases. This means schemas can be specified as
<db name>.<schema name>
, e.g.,"my db".main
. However, when such a schema name is specified, it gets quoted as"my db.main"
instead of"my db"."main"
. This PR fixes that by using thequote_schema
method of theidentifier_preparer
instead ofquote
. This method defaults toidentifier_preparer.quote
is overridden in the DuckDB sqlalchemy driver to parse the string and add quotes in the right places.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION