Add support for len function in query language #3756
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@mangalaman93 you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Mostly looks good though I have some comments for you @mangalaman93.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mangalaman93 and @manishrjain)
gql/parser.go, line 1270 at r2 (raw file):
buf.WriteString("count(") } else if f.Func.IsValueVar { buf.WriteString("val(")
This seems to be a new addition. Are we supporting something new here for value variables?
gql/parser.go, line 1571 at r2 (raw file):
} else if nestedFunc.Name == lenFunc { if len(nestedFunc.NeedsVar) > 1 { return nil, itemInFunc.Errorf("Multiple variables not allowed in a function")
Should we say in len function because they are allowed inside uid function.
query/query.go, line 2079 at r2 (raw file):
return } } else if sg.SrcFunc != nil && isInequalityFn(sg.SrcFunc.Name) && sg.SrcFunc.IsLenVar {
Could we merge the first two parts of this condition with the one above into the else condition?
} else if sg.SrcFunc != nil && isInequalityFn(sg.SrcFunc.Name) {
if sg.SrcFunc.IsValueVar {
} else if sg.SrcFunc.IsLenVar {
}
}
query/query.go, line 2085 at r2 (raw file):
if err != nil { // TODO(Aman): needs to do parent check? rch <- errors.Errorf("Invalid argment %v. Comparing with different type", val)
We seem to be throwing away the actual error here, should be using errors.Wrap to preserve that. Also it should be argument.
query/query0_test.go, line 1735 at r2 (raw file):
} // gt(pred, len(v)) // NOT OK
Should we add tests for the non OK cases in the parser and then remove these comments before merging?
There was a problem hiding this comment.
Changes here look good to me on a whole. There is a lot going on in these parsers so I'm glad you have good test coverage. Overall readability is definitely improved in this PR, nice job 👍
Reviewed with ❤️ by PullRequest
query/query.go
Outdated
| dst, err := types.Convert(src, types.IntID) | ||
| if err != nil { | ||
| // TODO(Aman): needs to do parent check? | ||
| rch <- errors.Errorf("Invalid argment %v. Comparing with different type", val) |
| value = "val" | ||
| typ = "type" | ||
| uidFunc = "uid" | ||
| valueFunc = "val" |
There was a problem hiding this comment.
Thanks for updating these other constants to have Func a suffix to match with lenFunc. Otherwise value, uid, typ also read like they would be local scope variables but this helps clear things up 👍
gql/parser.go
Outdated
| "within another. Got: %s", nestedFunc.Name) | ||
| } else if nestedFunc.Name == lenFunc { | ||
| if len(nestedFunc.NeedsVar) > 1 { | ||
| return nil, itemInFunc.Errorf("Multiple variables not allowed in a function") |
There was a problem hiding this comment.
Could consider adding a test for this error case in the parser_test.go
pawanrawal
changed the title
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @mangalaman93 and @pullrequest[bot])
gql/parser.go, line 1571 at r3 (raw file):
} else if nestedFunc.Name == lenFunc { if len(nestedFunc.NeedsVar) > 1 { return nil, itemInFunc.Errorf("Multiple variables not allowed in a function")
100 chars.
gql/parser.go, line 1571 at r3 (raw file):
Previously, pullrequest[bot] wrote…
Could consider adding a test for this error case in the parser_test.go
Yes, please do add a test for this case.
Also, add a test for uid(len(var)).
query/query.go, line 2080 at r3 (raw file):
} } else if sg.SrcFunc != nil && isInequalityFn(sg.SrcFunc.Name) && sg.SrcFunc.IsLenVar { val := sg.SrcFunc.Args[0].Value
Ensure that we have checked that Args len is greater than zero.
There was a problem hiding this comment.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
gql/parser.go, line 1270 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
This seems to be a new addition. Are we supporting something new here for value variables?
Realised that this function is only for debugging so its ok.
gql/parser.go, line 1571 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Should we say
in len functionbecause they are allowed insideuidfunction.
Done
gql/parser.go, line 1571 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Yes, please do add a test for this case.
Also, add a test for
uid(len(var)).
Done for both
gql/parser.go, line 1571 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done
query/query.go, line 2079 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Could we merge the first two parts of this condition with the one above into the else condition?
} else if sg.SrcFunc != nil && isInequalityFn(sg.SrcFunc.Name) { if sg.SrcFunc.IsValueVar { } else if sg.SrcFunc.IsLenVar { } }
Done
query/query.go, line 2085 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
We seem to be throwing away the actual error here, should be using
errors.Wrapto preserve that. Also it should beargument.
Done
query/query.go, line 2080 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Ensure that we have checked that Args len is greater than zero.
Done
query/query0_test.go, line 1735 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Should we add tests for the non OK cases in the parser and then remove these comments before merging?
Done
This allows filtering subgraph using the length of a uid list stored in uid variables. This change is the first step in supporting upsert functionality. (cherry picked from commit 800b443)
This change is