-
Notifications
You must be signed in to change notification settings - Fork 77
Support for T-SQL #313
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
Support for T-SQL #313
Conversation
Fix the module path in binding_test.go to fix issues that occur when installing the bindings from golang
Add optional database name to the object_reference rule and add some tests to add the new functionality
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.
please consider splitting this into two pull requests -- you've got a grab bag of fairly simple, straightforward improvements to t-sql support that are easy enough to evaluate on their own merits, but procedural sql and control flow is a longstanding known gap that has much bigger ramifications for dialect support and performance.
also, needs more tests :)
| ), | ||
|
|
||
| column_definition: $ => seq( | ||
| column_definition: $ => prec.left(seq( |
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.
this is a lot of "just prec.left it" and it's making me nervous
| optional(choice($.keyword_cascade, $.keyword_restrict)), | ||
| ), | ||
|
|
||
| drop_function: $ => seq( |
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'm pretty surprised we missed this, good catch!
| ), | ||
|
|
||
|
|
||
| tsql_parameter: $ => /@([a-zA-Z_][0-9a-zA-Z_]*)/, |
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.
if my hazy memory of t-sql is anything to go by this might be more an identifier semantically, and making it an option for that would also fit a lot better with what you're doing in function_argument and _qualified_field
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.
Yes that's my impression too, I wanted confirmation from your part first.
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.
@stackmystack I'm curious, have you found a way to introduce bracketed identifiers?
Great, I will do so in a separate PR
I am proposing these changes because I am pretty interested in procedural SQL and control flow. For the time being in T-SQL. I am putting the effort because I need them now. Would you be open to integrating them in
Definitely :) |
Hello Derek,
I would like to add support for Microsoft's T-SQL if possible.
This is a sketch of what it would look like.
I am not happy with the PR's state at all, but I thought I'd share what I have as of now, and collect feedback. I will re-write once, and if, support for T-SQL is accepted, and we agree on how to organise things; I am not proficient with all of the dialects of SQL so I may be implementing things in a stupid way.
The most important thing I would like to see, is recognising
@variable_namefrom T-SQL as anidentifierinstead of having it intsql_parameteras I do right now.This PR is motivated by a real-world use case, which I can include in the test suite once I figure out how to make it work :) (node-gyp on my mac is not happy):