You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2024/10/10 23:33:08 UTC
[PR] fix: `sqlparse` fallback for formatting queries [superset]
betodealmeida opened a new pull request, #30578:
URL: https://github.com/apache/superset/pull/30578
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
An improvement to https://github.com/apache/superset/pull/30350.
Today, when a DB engine spec has no corresponding `sqlglot` dialect, we use a generic one to parse queries, which works fine. When converting the AST back to a string to pretty-print SQL, though, we hit a problem: in some databases the query becomes invalid. For example, this Firebolt query:
```sql
SELECT col FROM t WHERE col NOT IN (1, 2);
```
Gets formatted to:
```sql
SELECT col FROM t WHERE NOT col IN (1, 2);
```
While equivalent, the latter is invalid in Firebolt.
The long term solution is to add a Firebolt dialect to `sqlglot`, at least handling this edge case. As a more generic solution, this PR implements logic where, when a DB engine spec doesn't have a corresponding dialect in `sqlglot`, the query is pretty-printed using `sqlparse.format`. This way we have the strictness and correctness of `sqlglot` when parsing queries, and the leniency of `sqlparse` when formatting them.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Added unit tests covering the problem.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: `sqlparse` fallback for formatting queries [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #30578:
URL: https://github.com/apache/superset/pull/30578#issuecomment-2407919084
@michael-s-molina I agree, I think ideally we want to have a close relationship with `sqlglot` (since we need to have a robust parser in order to enforce DAR and RLS reliably), so I hope we'll able to quickly address problems like this in the future by contributing upstream — creating a Firebolt dialect instead of relying on `sqlparse`, eg.
I'll add the deprecation notice. For 5.0 we can simply return the query as-is for DB engine specs without a `sqlglot` dialect.
--
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: `sqlparse` fallback for formatting queries [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #30578:
URL: https://github.com/apache/superset/pull/30578#discussion_r1797052236
##########
superset/sql/parse.py:
##########
@@ -138,24 +139,22 @@ class BaseSQLStatement(Generic[InternalRepresentation]):
"""
Base class for SQL statements.
- The class can be instantiated with a string representation of the script or, for
- efficiency reasons, with a pre-parsed AST. This is useful with `sqlglot.parse`,
- which will split a script in multiple already parsed statements.
+ The class should be instantiated with a string representation of the script and, for
+ efficiency reasons, optionally with a pre-parsed AST. This is useful with
+ `sqlglot.parse`, which will split a script in multiple already parsed statements.
The `engine` parameters comes from the `engine` attribute in a Superset DB engine
spec.
"""
def __init__(
self,
- statement: str | InternalRepresentation,
+ statement: str,
engine: str,
+ ast: InternalRepresentation | None = None,
):
- self._parsed: InternalRepresentation = (
- self._parse_statement(statement, engine)
- if isinstance(statement, str)
- else statement
- )
+ self._sql = statement
Review Comment:
Before this PR the `Statement` classes didn't store a reference to the original SQL, only the AST. So I had to modify it here.
--
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: `sqlparse` fallback for formatting queries [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #30578:
URL: https://github.com/apache/superset/pull/30578#issuecomment-2407883270
@betodealmeida We discussed this PR during the daily PR review meeting, and we think we should aim for removing any dependency of `sqlparse` in accordance to [[SIP-117] Improve SQL parsing](https://github.com/apache/superset/issues/26786). This means that we prefer to remove formatting for these databases in favor of removing `sqlparse`. That being said, we understand that this would constitute a breaking change given that this feature exists in 4.0 for these databases. We agreed on keeping your solution but adding a deprecation annotation/comment to the code and remove the `sqlparse` dependency in 5.0 even if the dialects are not correctly supported by then. I added the following [ticket](https://github.com/orgs/apache/projects/345?pane=issue&itemId=83111151) to the 5.0 board.
cc @villebro @justinpark @sadpandajoe
--
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: `sqlparse` fallback for formatting queries [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #30578:
URL: https://github.com/apache/superset/pull/30578
--
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