-
Notifications
You must be signed in to change notification settings - Fork 758
generate different sql code based on dialect #6915
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| formatSelectClause: defaultFormatter.formatSelectClause, | ||
| }, | ||
| default: defaultFormatter, | ||
| }; |
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.
i wonder if we can instead use guessDialect and the information from there for quoting
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.
thanks for this, it only works for bigquery as some dialects from codemirror don't have identifierQuotes.
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.
do we need the early returns for knowndialect? It seems like that could be error prone if we forget to add a case to the switch
| } | ||
|
|
||
| return `_df = mo.sql(f"SELECT ${columnName} FROM ${tableName} LIMIT 100", engine=${engine})`; | ||
| return `_df = mo.sql(f"""${selectClause}""", engine=${engine})`; |
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.
maybe new lines around the query in case it ends with a "
| switch (connection.dialect) { | ||
| ): SQLDialect { | ||
| const dialect = connection.dialect; | ||
| if (!isKnownDialect(dialect)) { |
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.
would it better to just fall though the switch-case so this does not get out of sync?
I can add a logNever and force us to return something. The reason to return early is to ensure the dialect is type-safe, so we must catch that dialect in the switch statements. It will throw error if spelling is off etc. |
📝 Summary
Fixes #6875. Might also tackle #6872.
🔍 Description of Changes
📋 Checklist