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 T-SQL DECLARE statement parsing #3462

Merged
merged 26 commits into from May 15, 2024

Conversation

jlucas-fsp
Copy link
Contributor

This PR adds DECLARE statement parsing to the TSQL dialect of SQLGlot. I haven't implemented any sql generation functionality for the new Declare and DeclareItem expression types, so running expression.sql will not work (let me know if this should be implemented before this PR could be merged)

This feature can parse scalar DECLAREs:
"DECLARE @var INT",
"DECLARE @var INT = 1",
"DECLARE @var1 INT, @var2 varchar(2)",
"DECLARE @string varchar(2) = 'AB'",
"DECLARE @var INT = (SELECT num FROM table WHERE condition)"

And, it can parse variable table DECLAREs:
"
DECLARE @myTable TABLE (
ID INT NOT NULL,
CONSTRAINT PK_ID PRIMARY KEY CLUSTERED (ID)
)
"

I didn't add any tests for this code to the existing test suite because the existing architecture seems to rely on parsing raw SQL into an AST, then converting the AST back into SQL to compare for accuracy, and this code can't convert the AST back into SQL yet.

Thank you all so much for all the work you've done on this library!

@tobymao
Copy link
Owner

tobymao commented May 11, 2024

this won’t be accepted without tests. do you not plan to add any?

@jlucas-fsp
Copy link
Contributor Author

@tobymao, Is there any way to test this without implementing sql generation features as well? Or, do you recommend I implement generation features anyway and then proceed with making tests?

@tobymao
Copy link
Owner

tobymao commented May 11, 2024

i’m not sure what you’re trying to achieve, you need to implement both parsing and generation

closing this for now and we can discuss

@tobymao tobymao closed this May 11, 2024
@jlucas-fsp
Copy link
Contributor Author

I've implemented SQL generation functionality and added tests for this (all passing on my machine). Those changes don't seem to automatically come to this PR, and I'm not sure what standard procedure would be in this case. I'm happy to open another PR with those new changes, but if you reopen this PR and those changes get automatically added here, that may be preferable.

To answer the implicit question in your previous message: My usecase for sqlglot doesn't involve transpilation or sql generation at all, it only involves the conversion of SQL to an AST. This is the first PR I've made to this repo and I wasn't sure exactly how much functionality around DECLAREs in TSQL I'd need to implement to get the parsing functionality in. Thank you for the reframe, and for future PRs, I now know what's expected.

@tobymao tobymao reopened this May 12, 2024
@tobymao
Copy link
Owner

tobymao commented May 12, 2024

i’ve reopened the pr, we can take a look at what you’ve done

@tobymao
Copy link
Owner

tobymao commented May 12, 2024

in order for this or to be accepted, please link all documentation of declare, all forms need to be implemented or fall back to command, otherwise this pr would be a regression

@jlucas-fsp
Copy link
Contributor Author

Here is the documentation for TSQL DECLARE:
https://learn.microsoft.com/en-us/sql/t-sql/language-elements/declare-local-variable-transact-sql?view=sql-server-ver16

I covered all the examples from the documentation in my tests (the only unsupported feature is inline indexes and inline constraints not prefixed with the CONSTRAINT keyword). If the DECLARE statement doesn't perfectly align with the structure I'm expecting, or errors out, the whole statement falls back to being parsed as a command

Are there any other changes I should make?

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jlucas-fsp, I've left some comments.

sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
tests/dialects/test_tsql.py Show resolved Hide resolved
tests/dialects/test_tsql.py Outdated Show resolved Hide resolved
tests/dialects/test_tsql.py Show resolved Hide resolved
tests/dialects/test_tsql.py Outdated Show resolved Hide resolved
tests/dialects/test_tsql.py Outdated Show resolved Hide resolved
@georgesittas
Copy link
Collaborator

Let's add a test for DECLARE @cursor_variable_name CURSOR as well.

Copy link
Contributor Author

@jlucas-fsp jlucas-fsp left a comment

Choose a reason for hiding this comment

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

Resolved most of the suggestions, but there are some suggestions I'm unsure about how to satisfy

sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Show resolved Hide resolved
tests/dialects/test_tsql.py Show resolved Hide resolved
tests/dialects/test_tsql.py Show resolved Hide resolved
sqlglot/expressions.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Show resolved Hide resolved
@georgesittas
Copy link
Collaborator

Thanks for addressing these comments @jlucas-fsp, I'll take another look shortly.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

@jlucas-fsp a few final comments. Regarding the hack / cleaning up part, don't worry about it. Once these comments are addressed I'll do another pass to try and clean it up 👍

tests/dialects/test_tsql.py Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/expressions.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
sqlglot/dialects/tsql.py Outdated Show resolved Hide resolved
jlucas-fsp and others added 8 commits May 15, 2024 12:28
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
@jlucas-fsp
Copy link
Contributor Author

@jlucas-fsp a few final comments. Regarding the hack / cleaning up part, don't worry about it. Once these comments are addressed I'll do another pass to try and clean it up 👍

Thank you so much for your detailed feedback on this PR, I very much appreciate it :)

@georgesittas georgesittas merged commit 54e31af into tobymao:main May 15, 2024
6 checks passed
@georgesittas
Copy link
Collaborator

@jlucas-fsp check out 49f7f85

@jlucas-fsp jlucas-fsp deleted the tsql_declare branch May 15, 2024 20:09
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.

None yet

4 participants