You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "guenp (via GitHub)" <gi...@apache.org> on 2023/12/14 22:55:30 UTC
[PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
guenp opened a new pull request, #26281:
URL: https://github.com/apache/superset/pull/26281
### 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 the `quote_schema` method of the `identifier_preparer` instead of `quote`. This method defaults to `identifier_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
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated PR: https://github.com/Mause/duckdb_engine/pull/848
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #26281:
URL: https://github.com/apache/superset/pull/26281
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "guenp (via GitHub)" <gi...@apache.org>.
guenp commented on code in PR #26281:
URL: https://github.com/apache/superset/pull/26281#discussion_r1428393134
##########
superset/db_engine_specs/base.py:
##########
@@ -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
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26281:
URL: https://github.com/apache/superset/pull/26281#issuecomment-1857145759
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26281?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`fca5b82`)](https://app.codecov.io/gh/apache/superset/commit/fca5b829df31cc7266211745ac70522a54ee23ba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.18% compared to head [(`724c08a`)](https://app.codecov.io/gh/apache/superset/pull/26281?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 58.93%.
> Report is 13 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26281 +/- ##
===========================================
- Coverage 69.18% 58.93% -10.26%
===========================================
Files 1945 1945
Lines 75948 75950 +2
Branches 8458 8458
===========================================
- Hits 52546 44759 -7787
- Misses 21217 29006 +7789
Partials 2185 2185
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.63% <100.00%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.53% <100.00%> (-21.33%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/26281/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <50.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26281?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #26281:
URL: https://github.com/apache/superset/pull/26281#discussion_r1427470010
##########
superset/db_engine_specs/base.py:
##########
@@ -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
Review Comment:
Daydreaming: How I wish Python had this syntax like JS/TS does..
```ts
{ quote, quote_schema } = engine.dialect.identifier_preparer
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "guenp (via GitHub)" <gi...@apache.org>.
guenp commented on PR #26281:
URL: https://github.com/apache/superset/pull/26281#issuecomment-1864893035
Please merge this PR. We need it to address this issue: https://github.com/apache/superset/issues/26286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sql lab): Use quote_schema instead of quote method to format schema name [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26281:
URL: https://github.com/apache/superset/pull/26281#discussion_r1427474860
##########
superset/db_engine_specs/base.py:
##########
@@ -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
Review Comment:
TIL I learned that `quote_schema` was a thing. Do we need to use this everywhere we use the `quote` method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org