node-sql-query icon indicating copy to clipboard operation
node-sql-query copied to clipboard

Adding TDS (aka Microsoft SQL) to dialects.

Open rdhammond opened this issue 12 years ago • 4 comments

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 avatar Aug 09 '13 02:08 rdhammond

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 avatar Aug 09 '13 03:08 rdhammond

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.

rdhammond avatar Aug 09 '13 04:08 rdhammond

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

dresende avatar Aug 10 '13 02:08 dresende

No worries. Thanks for taking a look.

rdhammond avatar Aug 10 '13 03:08 rdhammond