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

Only convert Scientific to Integer for nonnegative exponents #29

Merged
merged 3 commits into from
May 12, 2020

Conversation

abooij
Copy link
Contributor

@abooij abooij commented May 11, 2020

We use attoparsec's scientific number parsing code, which gives us a Scientific value. We like to work with Integers when we can, and so we sometimes convert this scientific number to an integer by simply taking its floor. But simply checking if the input text has a period symbol . in it does not suffice to check for correctness of this conversion, as numbers with a negative exponent may not be integral. This fixes that to only store numbers as integers if they have been supplied with a nonnegative exponent.

hasura/graphql-engine#4733 will be fixed by this (after we update the version of graphql-parser-hs used there).

@abooij
Copy link
Contributor Author

abooij commented May 11, 2020

I should add that another option is to check whether the input expression is in scientific notation, and decide based on that, rather than depending on the actual value. In the version implemented in this PR, an input value of, say, 8E3 is saved as the Integer value 8000. Under the alternative scheme, 8E3 would be stored as a Scientific value. Perhaps this is more true to "scientific notation" interpretation of the Scientific type.

I leave it up to the reviewer to make a decision on this.

@abooij abooij requested a review from 0x777 May 12, 2020 08:09
@abooij abooij merged commit 561580a into hasura:master May 12, 2020
abooij pushed a commit to abooij/graphql-engine that referenced this pull request May 12, 2020
abooij pushed a commit to abooij/graphql-engine that referenced this pull request May 12, 2020
abooij pushed a commit to hasura/graphql-engine that referenced this pull request May 13, 2020
shahidhk pushed a commit to hasura/graphql-engine that referenced this pull request May 29, 2020
nicuveo pushed a commit to nicuveo/graphql-parser-hs that referenced this pull request Jul 19, 2020
stevefan1999-personal pushed a commit to stevefan1999-personal/graphql-engine that referenced this pull request Sep 12, 2020
karthikvt26 pushed a commit to karthikvt26/graphql-engine that referenced this pull request Nov 17, 2020
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.

2 participants