-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Mssql decimal fix #3910
Conversation
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. |
I tried to store the following decimal value with knex and MSSQL: 10000000000005.74 |
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? 🤔 |
Because javascript numbers are actually float values, the check is not necessary... I will remove it. |
Cannot it be fixed in tedious if the root cause is there? |
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 |
I don't think so... see tediousjs/tedious#1058 |
You can use my added unit test to compare the master version with my fix. |
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.
All comments implemented
Everything else seems to work just fine, except for typescript tests ( |
The failing tests seem not to be caused by my changes. |
@hansnull Could you please pull latest changes from master? |
Fetch from knex master
a1c1060
to
b34b787
Compare
Done |
Released in 0.21.7 |
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