Fix panic in fillVars #3505
Fix panic in fillVars #3505
Conversation
There was a problem hiding this comment.
Just some minor comments.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @pawanrawal)
query/query.go, line 346 at r1 (raw file):
} func emptyIneqFnWithVar(sg *SubGraph) bool {
minor: maybe rename this to isEmpty... so that it's clear from the name that the function returns a bool.
query/query.go, line 1665 at r1 (raw file):
which could be run
change to something like "which were able to run" or "which were successfully run"
"which could be run" is a bit ambiguous. It might be interpreted as "which might be run", meaning the other queries have not been processed yet.
query/query.go, line 1716 at r1 (raw file):
return errors.Errorf("Wrong variable type encountered for var(%v) %v.", v.Name, v.Typ) default:
maybe add a glog.V(2) statement to track this branch was reached.
There was a problem hiding this comment.
@pawanrawal can you look at other changes like these which were trying to fix the broken-ness introduced by the original discussed change.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mangalaman93, @martinmr, and @pawanrawal)
query/query.go, line 1716 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
maybe add a glog.V(2) statement to track this branch was reached.
Please don't add glog output on queries in anything under glog.V(3).
There was a problem hiding this comment.
Sure @manishrjain, will have a look at other usages too. I have addressed the comments and am merging this change. Will open a separate PR against release/v1.0.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @mangalaman93 and @martinmr)
query/query.go, line 1665 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
which could be runchange to something like "which were able to run" or "which were successfully run"
"which could be run" is a bit ambiguous. It might be interpreted as "which might be run", meaning the other queries have not been processed yet.
Done.
query/query.go, line 1716 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Please don't add glog output on queries in anything under glog.V(3).
Done.
This fixes #3470. Test cases have been added to verify the fix. The default case in fillVars seemed hacky because it was assigning a value corresponding to uid 0 in uidToVal, that has been removed and appropriate changes have been made at other places.
This fixes dgraph-io#3470. Test cases have been added to verify the fix. The default case in fillVars seemed hacky because it was assigning a value corresponding to uid 0 in uidToVal, that has been removed and appropriate changes have been made at other places.
This fixes #3470. Test cases have been added to verify the fix. The default case in fillVars seemed hacky because it was assigning a value corresponding to uid
0inuidToVal, that has been removed. The code in the query package needs more work to clarify what the invariants are but that would be part of another PR.This change is