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

Adding TDS (aka Microsoft SQL) to dialects. #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Adding TDS (aka Microsoft SQL) to dialects. #18

wants to merge 4 commits into from

Conversation

rdhammond
Copy link

Hello! I'm working on an ETL project that uses node-orm2, which utilizes your query library. I would like this project to be able to talk to Microsoft SQL via Tedious to get the widest SQL coverage available. In order to do that, I need node-orm2 to talk to MSSQL---and in order to do /that/, I need node-sql-query to speak its dialect.

Since the library itself is pretty well organized, it was fairly trivial to add a dialect for MSSQL. I'm submitting this pull request so that others might benefit. I've also created a unit test module for the new TDS dialect (based on MySql) and made sure it passes all checks.

Hopefully this will help you guys out. If there's anything you need from me to accept the request, please let me know. (Or if you just want to reject it outright, I guess that works too!)

@rdhammond
Copy link
Author

Ugh, sorry. Jumped the gun on this. MSSQL uses different query structures from the ones this library's based around, so it's not as simple as a new dialect file.

I'd still like to see if I can get this working, but if it looks like it's going to violate the spirit & structure of the original library, I'll scuttle the pull request myself.

@rdhammond
Copy link
Author

Okay, I've got something up that should adapt TDS without violating abstract-ness. There are few concessions I had to make to get TDS up and running, though:

  • To maintain compatibility with SQL2008R2 and back, I used ROW_NUMBER() subqueries to do offsets. For simplicity's sake, this means the row number will "leak through" to the final results as a column named p_RN. I don't think this is a big deal, personally, but I'll leave it up to you to decide.
  • Since ROW_NUMBER() in TDS requires ordering, if an Order() clause isn't given, it assumes a column name of "Id". This is a pretty big assumption, but I didn't know of any other clean way to do it short of throwing an exception and refusing to process.
  • DELETE will accept LIMIT. OFFSET and ORDER BY are out of the question on TDS without some complex subqueries and will flat be ignored. (I don't know if it's worth the hassle to do them, honestly.)

OFFSET/LIMIT on TDS will respect GROUP BY, etc. the same way other queries would. It's the same logic, I just moved it to earlier in the function.

This is a pretty big change to come in from just some jerk off the street, so if you're uncomfortable including it in master, I totally understand. If you'd like to go forward with it, the only real item I have left is to test the generated statements to make sure they actually run---I don't have access to SQL Server from home, or I would've done it already.

@dresende
Copy link
Owner

It looks ok. Give me some time to read it, I'm on vacation and it's late now.

@rdhammond
Copy link
Author

No worries. 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.

2 participants