-
Notifications
You must be signed in to change notification settings - Fork 572
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
Conversation
this won’t be accepted without tests. do you not plan to add any? |
@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? |
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 |
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. |
i’ve reopened the pr, we can take a look at what you’ve done |
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 |
Here is the documentation for TSQL DECLARE: 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? |
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 the PR @jlucas-fsp, I've left some comments.
Let's add a test for |
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.
Resolved most of the suggestions, but there are some suggestions I'm unsure about how to satisfy
Thanks for addressing these comments @jlucas-fsp, I'll take another look shortly. |
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.
@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 👍
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>
Thank you so much for your detailed feedback on this PR, I very much appreciate it :) |
@jlucas-fsp check out 49f7f85 |
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!