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

Mssql decimal fix #3910

Merged
merged 9 commits into from Sep 30, 2020
Merged

Mssql decimal fix #3910

merged 9 commits into from Sep 30, 2020

Conversation

hansnull
Copy link
Contributor

@hansnull hansnull commented Jul 2, 2020

Hi knex guys,

I have written a fix for large decimals in mssql, related to tediousjs/tedious#1058.
My fix adds dynamic scaling for decimal values and prevents an UInt64 overflow in tedious.

Best regards,
hansnull

lib/dialects/mssql/index.js Outdated Show resolved Hide resolved
@elhigu
Copy link
Member

elhigu commented Jul 2, 2020

Please try to explain super clearly what is the problem you are now trying to solve here. What do you mean by dynamic scaling and why it is needed / wanted.

@hansnull
Copy link
Contributor Author

hansnull commented Jul 2, 2020

I tried to store the following decimal value with knex and MSSQL: 10000000000005.74
No error was thrown, but MSSQL stores actually: -20037647.7924
The root cause for that is the following line:
https://github.com/tediousjs/tedious/blob/4120817754e91a32d9419093770771df590365ca/src/data-types/decimal.ts#L75
My fix calculates the scale and precision dynamically which fixes the problem for the number 10000000000005.74
To make sure that no other value overflows the UINT64 I added the check.

@elhigu
Copy link
Member

elhigu commented Jul 2, 2020

Is this trying to recognize that if there is bigint coming from DB and it is too big to store in javascript number that error should not be thrown? 🤔

@hansnull
Copy link
Contributor Author

hansnull commented Jul 2, 2020

Because javascript numbers are actually float values, the check is not necessary... I will remove it.

@elhigu
Copy link
Member

elhigu commented Jul 2, 2020

@hansnull
Copy link
Contributor Author

hansnull commented Jul 2, 2020

It is also related to knex, because the default for the scale is 10 and for precision 38, which is not needed for a number like 10.01

@hansnull
Copy link
Contributor Author

hansnull commented Jul 2, 2020

https://github.com/tediousjs/tedious/blob/4120817754e91a32d9419093770771df590365ca/src/data-types/decimal.ts#L75

Cannot it be fixed in tedious if the root cause is there?

I don't think so... see tediousjs/tedious#1058

@hansnull
Copy link
Contributor Author

hansnull commented Jul 2, 2020

You can use my added unit test to compare the master version with my fix.

lib/dialects/mssql/index.js Outdated Show resolved Hide resolved
test/integration/datatype/decimal.js Show resolved Hide resolved
lib/dialects/mssql/index.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@hansnull hansnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments implemented

@elhigu
Copy link
Member

elhigu commented Jul 18, 2020

Everything else seems to work just fine, except for typescript tests (npm run lint:types && npm run lint)seems to be failing for some reason.

@hansnull
Copy link
Contributor Author

The failing tests seem not to be caused by my changes.

@kibertoad
Copy link
Collaborator

@hansnull Could you please pull latest changes from master?

Fetch from knex master
@hansnull
Copy link
Contributor Author

@hansnull Could you please pull latest changes from master?

Done

@kibertoad kibertoad merged commit 8029396 into knex:master Sep 30, 2020
@kibertoad
Copy link
Collaborator

Released in 0.21.7

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.

None yet

3 participants