Skip to content
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

Feat: add mssql and sqlserver as aliases of the TSQL dialect #4666

Closed
wants to merge 1 commit into from

Conversation

georgesittas
Copy link
Collaborator

No description provided.

@barakalon
Copy link
Collaborator

What's the reason here?

@georgesittas
Copy link
Collaborator Author

georgesittas commented Jan 28, 2025

What's the reason here?

Seems like it's common for folks to use MSSQL, T-SQL and SQL Server interchangeably to refer to the same dialect (relevant SO thread). In SQLMesh we refer to the engine as MSSQL but since the dialect here is called TSQL, you can't do stuff like sqlmesh init mssql.

You could argue that the dialect is tsql though, so this isn't necessarily needed. Just a quality-of-life type of thing. Thought I'd just put this up for discussion, happy to close if y' all disagree.

Copy link
Collaborator

@VaggelisD VaggelisD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I also think it's a nice-to-have UX feature e.g Postgres could also have psql as an alias etc, up to Toby & Barak to decide

@barakalon
Copy link
Collaborator

The dialect names are unique identifiers.
If this is a UX concern for sqlmesh, maybe sqlmesh should handle normalizing dialect names?

@barakalon
Copy link
Collaborator

I just worry this adds as much complexity/confusion as it does convenience.
And it doesn't really solve the discovery problem for sqlglot users, since the list of dialects doesn't include mssql or sqlserver. So users still need to know about tsql? I.e., I don't think we support users just guessing the names of dialects in sqlglot haha

@georgesittas
Copy link
Collaborator Author

I'm persuaded 👍
@tobymao agrees this shouldn't go in here as well, fwiw.

Thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants