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

[BUG] Hasura doesn't calculate correctly small numbers _inc #4733

Closed
perrosnk opened this issue May 11, 2020 · 16 comments · Fixed by #4741
Closed

[BUG] Hasura doesn't calculate correctly small numbers _inc #4733

perrosnk opened this issue May 11, 2020 · 16 comments · Fixed by #4741
Assignees
Labels
k/bug Something isn't working

Comments

@perrosnk
Copy link

If you use a number less than 0.000001 (6 decimals) which javascript converts it to 1E-7 hasura doesn't calculate correctly the _inc value.

This has caused a lot of trouble to us. Could we expect a fix?

@abooij
Copy link
Contributor

abooij commented May 11, 2020

What postgres type are you using to store these values? And can you please also show which values you are adding up (including before, after, and expected values)?

Note that the real postgres type is mapped to a 4 byte IEEE 754 float, which doesn't have more than 6 decimals of precision: so for these types, _incing a small value such as 0.000001 can, by the very definition of IEEE 754, be forced to be the same as adding 0, and this should be calculated by graphql-engine correctly. So this would not be a mistake of graphql-engine, but rather a limitation of IEEE 754.

(Worse: adding two real values of dramatically different order of magnitude may mean simply returning one of the two numbers: for instance, 1E8+1=1E8 rather than 1.00000001E8. See also this IEEE 754 calculator - click "binary32" at the bottom of the page for a 4 byte precision float. Indeed, floats are problematic in all software, but graphql-engine should not introduce any additional problems.)

@abooij abooij self-assigned this May 11, 2020
@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@abooij I am using numeric and I need values down to 1E-8

@abooij
Copy link
Contributor

abooij commented May 11, 2020

@perrosnk Could you please show which values you are adding up (including before, after, and expected values)?

@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@abooij I am actually trying to decrement a value using _inc: {balance: ${-amount}}.
This amount variable can be a floating point down to 1E-8. Hasura mistakenly substacts 1 instead of 0.00000001

@abooij
Copy link
Contributor

abooij commented May 11, 2020

@perrosnk Sorry, I'm really going to need some more details in order to reproduce this issue. Can you please show a sequence of GraphQL queries and their corresponding outputs, indicating precisely where you believe Hasura to be mistaken?

@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@abooij I have a model User and a column balance of type numeric.

I want to decrement the balance using an update_user_by_pk mutation.
The amount variable shown above is also a Float number, and can be greater or equal to 0.00000001 (8 decimal point).

The amount is a variable sent by the user, and is 1E-8.
When I use the code

update_users_by_pk(pk_columns: {id: "${senderId}"}, _inc: {balance: ${-amount}}) {
    id
  }

hasura mistakenly decrements the balance by 1 and not 0.00000001

By the way, the variable amount passed to the mutation is not a string, it's a number.

Was that information enough or do you need any further information? Please let me know

Edit: senderId and amount are both javascript variables from the code:

const senderId = "alonguuid"
const amount = 1E-8

@tirumaraiselvan
Copy link
Contributor

Hi @perrosnk I guess it is getting clearer what the issue is. You are trying to send the literal 1E-8 as a value to the numeric field?

Since this is not an actual valid graphql query:

update_users_by_pk(pk_columns: {id: "${senderId}"}, _inc: {balance: ${-amount}}) {
    id
  }

because the variable usage is not graphql syntax. Could you send the final query that is being sent to the server?

@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@tirumaraiselvan @abooij Here is the actual graphql string:

      mutation MyMutation {
        update_users_by_pk(pk_columns: {id: "aLongUUID"}, _inc: {balance: -1e-8}) {
          id
        }
      }

@abooij
Copy link
Contributor

abooij commented May 11, 2020

Thanks for reporting this, @perrosnk. I have found the origin of this issue. I will send in a PR to fix this today or tomorrow.

@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@abooij Could this be released to npm as a patch version (ex.1.2.2) or do we need to wait for the next release?

@abooij
Copy link
Contributor

abooij commented May 11, 2020

@perrosnk As a workaround, you can either pass the value as a string, or convert it to non-scientific notation. In other words, the following queries should work:

mutation MyMutation {
  update_users_by_pk(pk_columns: {id: "aLongUUID"}, _inc: {balance: "-1e-8"}) {
    id
  }
}
mutation MyMutation {
  update_users_by_pk(pk_columns: {id: "aLongUUID"}, _inc: {balance: 0.00000001}) {
    id
  }
}

@perrosnk
Copy link
Author

@abooij I tested the option of "-1e-8" and it doesn't work. I am not sure if I can test the other option because javascript makes the number automatically 1E-8. I should use number.toFixed(8), but this returns a string too.

@abooij
Copy link
Contributor

abooij commented May 11, 2020

@perrosnk I'm sorry to hear the workaround doesn't work for you. Make sure you are using the quote symbols, so passing the value as "-1E-8" rather than -1E-8 in the actual GraphQL string (the format you posted here).

In any case, I have prepared a fix for this, so hopefully this will be part of graphql-engine soon.

@perrosnk
Copy link
Author

perrosnk commented May 11, 2020

@abooij I passed "-1E-8" but it doesn't work. It doesn't work even if I pass "0.00000001" (as a string). I can't pass a decimal number 0.00000001 because javascript automatically converts it to 1E-8

@perrosnk
Copy link
Author

perrosnk commented May 15, 2020

@abooij Thank you very much for this! When may we expect this to be released?

@abooij
Copy link
Contributor

abooij commented May 18, 2020

@perrosnk Keep an eye out for the latest releases, hopefully this fix will be released soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants