-
Notifications
You must be signed in to change notification settings - Fork 16.7k
fix: Use singlestoredb dialect for sqlglot #36096
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| Dialect, | ||
| Dialects, | ||
| ) | ||
| from sqlglot.dialects.singlestore import SingleStore | ||
| from sqlglot.errors import ParseError | ||
| from sqlglot.optimizer.pushdown_predicates import ( | ||
| pushdown_predicates, | ||
|
|
@@ -101,7 +102,7 @@ | |
| "redshift": Dialects.REDSHIFT, | ||
| "risingwave": Dialects.RISINGWAVE, | ||
| "shillelagh": Dialects.SQLITE, | ||
| "singlestore": Dialects.MYSQL, | ||
| "singlestoredb": SingleStore, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing backward compatibility for SingleStore engine name
Tell me moreWhat is the issue?The mapping key changed from 'singlestore' to 'singlestoredb' but there's no backward compatibility handling for existing configurations that may still use 'singlestore'. Why this mattersExisting Superset installations with SingleStore databases configured using the 'singlestore' engine name will suddenly fail to parse SQL correctly, as they will fall back to the default 'base' dialect instead of the proper SingleStore dialect. This could cause SQL parsing errors and incorrect query behavior. Suggested change ∙ Feature PreviewAdd both mappings to maintain backward compatibility: "singlestore": SingleStore,
"singlestoredb": SingleStore,This ensures existing configurations continue to work while supporting the new naming convention. Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| "snowflake": Dialects.SNOWFLAKE, | ||
| # "solr": ??? | ||
| "spark": Dialects.SPARK, | ||
|
|
||
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.
Unnecessary module-level import overhead
Tell me more
What is the issue?
The import of SingleStore dialect class is added at module level, which means it will be imported every time this module is loaded, even when SingleStore database is not being used.
Why this matters
This creates unnecessary import overhead for all users of this module, regardless of whether they actually use SingleStore databases. The import will add to module loading time and memory usage even for applications that never interact with SingleStore.
Suggested change ∙ Feature Preview
Consider using lazy importing by moving the import inside the code path where it's actually needed, or use a factory pattern that only imports the dialect when the specific engine is requested. For example:
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
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.
Unsure why, but the
Singlestoredialect is not listed here (v27.15.2is the currentsqlglotpinned version in Superset), so we have to import the dialect directly. I checked the latestsqlglotversion too but it's still the case.