-
Notifications
You must be signed in to change notification settings - Fork 443
Add nvarchar(max), binary(n), varbinary(n/max) and decimal(s,p) support #3
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
Conversation
Provides a partial solution for handling of Partially Length-prefixed Byte data. Does not currently support 'unknown plp length' data (solely because I struggled to get a query to generate that type) Not really sure how one would support these types fully as I would imagine they break the nodejs buffer limits before we can return all the data we need (the buffer.position is 32bit, the maximum returnable data is around 64bit )
Utilises the same processing logic as the NumericN type. However I think this (and the existing) logic does not fully support the numeric range that could come back from the server i.e. if precision >=29 or precission <=38 then we need to be operating over 16 bytes, we definitely aren't :)
|
I made a rather large change to token parsing (f550296), that will make it somewhat difficult to do a git merge for these changes. So I think that I'm going to tackle them one data type at a time. For each data type, I'll take the gist of your changes, and apply them manually. |
|
No worries I separated out the test commits so you should be able to cherry
On Saturday, December 31, 2011, Mike D Pilsbury <
|
|
pekim/tedious/@d47f645a8553f9e6210d01ce383c383fb940ac55 - Added support for decimal type, including 12 and 16 byte numeric/decimal values |
|
Cool, so thats decimal support, I'm surprised that the >8byte wide values 'work' and don't truncate on the precision, why is that? |
|
12 and 16 byte precision values do indeed appear to work. I'm taking 32 bits at a time, using Buffer.readUInt32LE to create a Javascript number. The 2, 3 or 4 of those Numbers (for 8, 12, or 16 bytes) are multiplied together to make a (possibly slightly inaccurate) Number for the precison value. Although there may be some loss of accuracy due to the imprecision of Javascript floating point numbers, even after scaling, the result seems to be correct for the numbers I happen to have used in the tests. I'm sure that some values won't be so accurately represented, so there may sometimes be small differences between the value returned from the server and the value represented in Javascript. |
|
I think that I've now covered everything. |
|
Looks good. I just need/want Float and I'm good to export all my data outta SQL Server (my end-game!) .. I suspect for the varbinary(max) and nvarchar(max) it would be preferable to write out to a temp file and return a stream onto that (but for my usages I'm good I think ;) ) ... I'm still unsure how one would cope with resultsets > 2GB using a node.js buffer object (but I probably need to revise them!) |
|
I've added support for |
|
Looks good, thank you :) I can now happily return all the data types I need! Woohoo. |
Ae collation support test
My database appears to hit all your (presumably) known issues :)
Please find attached a pull request to add initial support for nvarchar(max) columns. I've implemented NULL and known length, but not 'unknown length' (I couldn't seem to get a query to generate column results like that.)
On a side note, is there not a fundamental issue with the node.js buffers for these large (64bit length) types of data ? Just not sure
how one would cope with that ?
I've extended this pull request to also include support for binary(n) and varbinary(n/max) types (the results are returned as a byte array, fixed width in the former case, variable width in the latter.)
I've extended it (again) to support 'decimal(s,p)' column types, these appear to be synonymous to numeric(s,p) types so I've piggy-backed on that implementation, I note that it won't support the full width of data that could be returned, I guess a full big integer library support is required after all :(