Skipping floats that cannot be marshalled #5163
Conversation
gja
changed the title
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)
query/outputnode.go, line 257 at r1 (raw file):
// +Inf, -Inf and NaN are not representable in JSON. // Please see https://golang.org/src/encoding/json/encode.go?s=6458:6501#L573 if math.IsInf(v.Value.(float64), 0) || math.IsNaN(v.Value.(float64)) {
Am I casting this the wrong way? This is how golang casts internally https://golang.org/src/reflect/value.go?s=27933:27963#L890
There was a problem hiding this comment.
Looks good from my end. Get it approved by @pawanrawal .
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gja and @pawanrawal)
query/outputnode.go, line 257 at r1 (raw file):
Previously, gja (Tejas Dinkar) wrote…
Am I casting this the wrong way? This is how golang casts internally https://golang.org/src/reflect/value.go?s=27933:27963#L890
Add some tests, approach looks good.
There was a problem hiding this comment.
Actually, can you remove the JIRA issue number from the top line. It's not something that our open source users can track. You can mention it in the description of the PR.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gja and @pawanrawal)
There was a problem hiding this comment.
One comment but otherwise it
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gja and @pawanrawal)
query/outputnode.go, line 257 at r1 (raw file):
v.Value.(float64)
Doing it this way, could trigger a panic if for some reason the value says it's type is a float but the actual value is not one. Not super likely I guess but better to be proactive.
The fix is easy, see https://tour.golang.org/methods/15.
query/outputnode.go
Outdated
| @@ -337,77 +343,55 @@ func (fj *fastJsonNode) encode(out *bytes.Buffer) error { | |||
| a.order = i | |||
| } | |||
|
|
|||
| // Scalar Value | |||
There was a problem hiding this comment.
please hide whitespace when going through this change.
query/query.go
Outdated
| @@ -216,6 +216,26 @@ type Function struct { | |||
|
|
|||
| // SubGraph is the way to represent data. It contains both the request parameters and the response. | |||
| // Once generated, this can then be encoded to other client convenient formats, like GraphQL / JSON. | |||
| // SubGraphs are recursively nested. Each SubGraph contain the following: | |||
| // * SrcUIDS: A list of UIDs that were processed by this query. If this subgraph is a child graph, then the | |||
| // DestUIDs of the parent must match the SrcUIDs of the childe. | ||
| // * DestUIDs: A list of UIDs for which there can be output found in the Children field | ||
| // * Children: A list of child results for this query | ||
| // * valueMatrix: A list of values, against a single attribute, such as name (for a scalar subgraph). |
| // * valueMatrix: A list of values, against a single attribute, such as name (for a scalar subgraph). | ||
| // This must be the same length as the SrcUIDs | ||
| // * uidMatrix: A list of outgoing edges. This must be same length as the SrcUIDs list. | ||
| // Example, say we are creating a SubGraph for a query "users", which returns one user with name 'Foo', you may get |
gja
changed the title
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gja)
query/query.go, line 220 at r2 (raw file):
// Once generated, this can then be encoded to other client convenient formats, like GraphQL / JSON. // SubGraphs are recursively nested. Each SubGraph contain the following: // * SrcUIDS: A list of UIDs that were processed by this query. If this subgraph is a child graph, then the
UIDs that were given as input to this query
query/query.go, line 221 at r2 (raw file):
// SubGraphs are recursively nested. Each SubGraph contain the following: // * SrcUIDS: A list of UIDs that were processed by this query. If this subgraph is a child graph, then the // DestUIDs of the parent must match the SrcUIDs of the childe.
children
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gja and @martinmr)
query/outputnode.go, line 257 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
v.Value.(float64)Doing it this way, could trigger a panic if for some reason the value says it's type is a float but the actual value is not one. Not super likely I guess but better to be proactive.
The fix is easy, see https://tour.golang.org/methods/15.
Yes, I agree here. Other way to achieve this would be a type switch as we have done for types.IntID.
| @@ -216,6 +216,26 @@ type Function struct { | |||
|
|
|||
| // SubGraph is the way to represent data. It contains both the request parameters and the response. | |||
| // Once generated, this can then be encoded to other client convenient formats, like GraphQL / JSON. | |||
| // SubGraphs are recursively nested. Each SubGraph contain the following: | |||
| // * SrcUIDS: A list of UIDs that were sent to this query. If this subgraph is a child graph, then the | |||
(cherry picked from commit d4cdf5e)
Explicitly checking for +Inf / -Inf / NaN when serializing floating points. This is the same thing go does internally.
Open Questions:
This change is
Docs Preview: