Spec should be more explicit about whether NaN and Inf are valid Floats #778
Comments
2 tasks
|
I encountered a similar problem in graphql-java regarding the range of Float and infinities. I also couldn't find a definite answer in the spec. graphql-java/graphql-java#1998 |
|
Does "fractional values" not exclude NaN and Infinity? Either way, we could definitely clarify that we only support finite values; I encourage you to send a PR and add it to the next GraphQL Working Group call. |
glasser
added a commit
to glasser/graphql-spec
that referenced
this issue
Sep 16, 2020
Fixes graphql#778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN".
glasser
added a commit
to glasser/graphql-spec
that referenced
this issue
Sep 16, 2020
Fixes graphql#778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN".
|
Yeah, please add it as a 5m topic to https://github.com/graphql/graphql-wg/blob/master/agendas/2020-10-01.md. I think it's fairly non-controversial, so as long as there's not too much bike-shedding it should be a quick approval. |
Enrico2
pushed a commit
to apollographql/federation
that referenced
this issue
Sep 18, 2020
This PR implements the Auto Fragmentization feature in the Rust Query Planner. Be sure to "ignore whitespace" when reviewing. ## Process Prior to this PR, we have copied over the scaffolding from the .ts implementation of the query planner to implement this feature later. In the .ts implementation, we discover and create "internal fragments" as we work through the query planner. However, as I started working at this problem, I decided that separating auto-frag to a processing phase that happens separately makes for simpler, more extensible code, that will allow us to iterate on this specific feature in a decoupled way from the rest of the query planner. The main query planning process creates `FetchNode`s, each of which has an `operation` field that contains the literal string that will be sent to the downstream process. This is where we want to use queries that contain fragments we've automatically discovered. When a query is planned with auto-frag enabled (a new flag to the `plan` function in `QueryPlanner`), right before we serialize the `operation` for the `FetchNode`, we _move_ the selection set through an auto fragmentization pass. The process recursively iterates through the selection set, where for each selection set that has more than 2 fields (see parity section below) we first check if we've seen this selection set before (see hashing section below). If we create a fragment if there is none, and replace the selection set with one that only has one selection, the fragment spread for the fragment that we just found/created. ### Parity, not optimization This implementation is not very intelligent, and the resulting query highly depends on the incoming query. For example, we may create many fragments that are only used once or we may not create fragments for actually really complex selection sets, e.g. selection sets with 2 selections whose sub-trees are deeply nested selection sets that repeat often. The goal for this initial implementation is to reach parity with the .ts implementation so that we could completely replace the .ts implementation with an invocation of this one via wasm, without affecting users who are relying on the existing implementation. Down the line, we can work on various optimizations for this code, as the need arises, or accept contributions to it from the community. (e.g. detecting fragments in subsets of selection sets? :)). ### Hashing In order to identify if we've seen a selection set before, to use as a key in a map, we need the ability to hash a selection set based on _content_. In order to get this hash value we had to do 2 things: 1. Limit the `f64` values to `NotNan` [using this crate](https://docs.rs/ordered-float/2.0.0/ordered_float/struct.NotNan.html). Rust cannot derive a `Hash` for `f64` because of NaN values. This is a whole beast you can read about [here](https://internals.rust-lang.org/t/f32-f64-should-implement-hash/5436). Looking at the GraphQL Spec, a Float literal is defined as ``` FloatValue :: IntegerPart FractionalPart IntegerPart ExponentPart IntegerPart FractionalPart ExponentPart ``` which as far as I can tell, from a _parsing_ perspective, any literal value cannot be a `NaN`, and thus, hashable. graphql-js doesn't allow this as well. [See this issue](graphql/graphql-spec#778) by @glasser 2. In order for the hash values to be based on _content_, the `Hash` implementations have to skip any field that references a _position_ for the element in the original query. I used the `derivative` crate for this (good find @trevor-scheer !!) to derive `Hash` and skip `position` and `span` fields. ## Important note about testing This code prints out the fragments in each operation in predictable order; sorted by fragment name. The tests we have in the `.feature` files have different ordering. We only have test cases with 1 or 2 fragments, so I can't really say if this is always "reverse" order, in any case it most likely depends on insertion order in the .ts implementation. But because this implementation is different, we can't resolve that by using a LinkedHashMap like in other cases before. And so I changed the relevant expected result in the .feature files to use the lexicographic ordering. This is the 2nd case where we'll have to adapt if we want to use the same feature files for different code bases. My recommendation would be to either make the other implementations' output have predictable ordering, or do the comparison in the test in a way that doesn't rely on ordering where it's not important. In this PR's case, if we wanted to make the test agnostic to fragment ordering, it'd mean parsing the `operation` in a FetchNode and comparing the parsed object, while _ignoring_ any position/span fields (which means we can't use the derived `PartialEq` unless we change that too), given the complexity of that, I opted to change the .feature file instead. Lmk if this is a problem. My one simple test case passed, but when enabling testing using the `.feature` files that have auto-frag, I saw some failures but had to go afk, I will debug these before this PR is opened as not a draft. ## Integration with Javascript This PR doesn't change the API used by the wasm module just yet. I figured we'd do that in a separate PR that involves a minor version update to the wasm crate.
leebyron
pushed a commit
to glasser/graphql-spec
that referenced
this issue
Apr 6, 2021
Fixes graphql#778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN".
leebyron
added a commit
that referenced
this issue
Apr 7, 2021
* Clarify that Float does not include NaN or infinity Fixes #778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN". * Update Section 3 -- Type System.md * Update CompleteValue() to not return early for NaN Co-authored-by: Lee Byron <[email protected]>
This was referenced Oct 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The spec documents the Float type as representing "signed double‐precision fractional values as specified by IEEE 754".
In addition to "normal" finite numbers, IEEE 754 includes NaN and positive and negative infinity.
A plain reading of section 3.5.2 would imply that the GraphQL Float type includes all IEEE 754 values, not just the finite ones.
However, I don't believe this is the intent. First, you cannot express these values as literals, which is very clear in the definition of FloatValue. Secondly, the reference implementation GraphQL-JS explicitly rejects non-finite values (introduced by @IvanGoncharov in graphql/graphql-js#1365 and @leebyron in graphql/graphql-js#1382).
The only allusion to NaN in the spec is in the value completion section which it suggests that mapping NaN to null may be appropriate (I don't believe graphql-js does this, though).
I think the right fix is to change the Float section to be clear that only IEEE 754 finite values are part of the Float type (and maybe to remove the note about NaN mapping to null). Happy to send a PR if folks agree.
The text was updated successfully, but these errors were encountered: