Add support for reading from and to arguments for shortest path query from variables. #3710
Conversation
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pawanrawal)
gql/parser.go, line 2430 at r1 (raw file):
gq.Func = gen gq.NeedsVar = append(gq.NeedsVar, gen.NeedsVar...) case "from", "to":
Does this really need changes to gql/parser? Or, can we do this variable name parsing after?
In general, better to move this to after.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pawanrawal)
gql/parser.go, line 94 at r1 (raw file):
} type ShortestPathArgs struct {
Add a doc string.
gql/parser.go, line 2388 at r1 (raw file):
if gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 && gq.ShortestPathArgs.From == nil && gq.ShortestPathArgs.To == nil {
This condition is getting a little out of hand. Move it into a boolean function and use
if functionName(gq) {
...
}
query/query.go, line 1702 at r1 (raw file):
fromVar) } if uidVar.Uids != nil {
can uidVar.Uids equal nil? What should happen in that case? I assume From (or To) is just 0 so it will be handled later but maybe a comment on why that is the case could be helpful here.
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @martinmr)
gql/parser.go, line 94 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Add a doc string.
Done.
gql/parser.go, line 2388 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
if gq.Func == nil && len(gq.NeedsVar) == 0 && len(gq.Args) == 0 && gq.ShortestPathArgs.From == nil && gq.ShortestPathArgs.To == nil {This condition is getting a little out of hand. Move it into a boolean function and use
if functionName(gq) { ... }
Done.
gql/parser.go, line 2430 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Does this really need changes to gql/parser? Or, can we do this variable name parsing after?
In general, better to move this to after.
As discussed earlier, right now its being parsed as a string and being set in gq.Args which is a map[string]string. We are changing it to a function so that it can take in the uid function as well.
query/query.go, line 1702 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
can
uidVar.Uidsequal nil? What should happen in that case? I assume From (or To) is just 0 so it will be handled later but maybe a comment on why that is the case could be helpful here.
Yes, it can be nil and that is handled in shortest.go. Added a comment.
There was a problem hiding this comment.
Reviewed 2 of 5 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @pawanrawal)
gql/parser.go, line 2465 at r2 (raw file):
its
it's
query/query.go, line 884 at r2 (raw file):
if gq.ShortestPathArgs.From == nil || gq.ShortestPathArgs.To == nil { return errors.Errorf("Unexpected error: from/to can't be nil for shortest path")
Don't need to write stuff like "Unexpected error" inside error messages. It's implied it's an error. So this should be:
return errors.Errorf("from/to can't be nil for shortest path")
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @pawanrawal)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pawanrawal)
gql/parser.go, line 2449 at r3 (raw file):
if key == "from" { gq.ShortestPathArgs.From = fn } else {
else if key == "to" ... else don't do anything or return an error?
gql/parser.go, line 2478 at r3 (raw file):
} return nil, item.Errorf("from/to in shortest path can only accept uid function or an uid. Got: %s",
100 chars.
query/query.go, line 1708 at r3 (raw file):
return errors.Errorf("from variable(%s) should only expand to 1 uid", fromVar) } sg.Params.From = uidVar.Uids.Uids[0]
uidVar.Uids.Uids sounds wrong. Maybe work on it in a separate PR.
query/query3_test.go, line 781 at r3 (raw file):
require.JSONEq(t, `{"data": {"_path_":[ {"uid":"0x1","_weight_":3,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3ea"}}}},
100 chars.
query/query3_test.go, line 782 at r3 (raw file):
`{"data": {"_path_":[ {"uid":"0x1","_weight_":3,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3ea"}}}}, {"uid":"0x1","_weight_":4,"path":{"uid":"0x1f","path":{"uid":"0x3e8","path":{"uid":"0x3e9","path":{"uid":"0x3ea"}}}}}],
100 chars.
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 7 unresolved discussions (waiting on @manishrjain and @martinmr)
gql/parser.go, line 2465 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
itsit's
Done.
gql/parser.go, line 2449 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
else if key == "to" ... else don't do anything or return an error?
Added else if key == "to" to make it explicit. This code is within a case with only from, and to so it should be good enough.
gql/parser.go, line 2478 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done.
query/query.go, line 884 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Don't need to write stuff like "Unexpected error" inside error messages. It's implied it's an error. So this should be:
return errors.Errorf("from/to can't be nil for shortest path")
Done.
query/query.go, line 1708 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
uidVar.Uids.Uids sounds wrong. Maybe work on it in a separate PR.
Will look at it in a separate PR.
query/query3_test.go, line 781 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done.
query/query3_test.go, line 782 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done.
Fixes #1243.
This change is