-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
return error if keywords used as alias in groupby #3725
Conversation
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
860dc04
to
2ccb745
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor comments.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @Sch00lb0y)
gql/parser.go, line 1952 at r1 (raw file):
} if validKey(val) { return item.Errorf("Can't use %s as alias.", val)
minor: no period at the end of error messages (to match the go convention for fmt.Errorf).
gql/parser_test.go, line 3028 at r1 (raw file):
query { me(func: uid(0x1)) { friends @groupby(first: 10, SchooL: school) {
minor: I'd write this as friends @groupby(first: school) {
. Otherwise it's unclear that first
is an alias for a predicate. Right now it looks like first is another argument passed to groupby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @Sch00lb0y)
gql/parser.go
Outdated
@@ -1948,6 +1948,9 @@ func parseGroupby(it *lex.ItemIterator, gq *GraphQuery) error { | |||
if alias != "" { | |||
return item.Errorf("Expected predicate after %s:", alias) | |||
} | |||
if validKey(val) { | |||
return item.Errorf("Can't use %s as alias.", val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say "Can't use keyword %s as alias in groupby."
What happened to this PR, @balajijinnah ? |
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Signed-off-by: பாலாஜி ஜின்னா balaji@dgraph.io
This change is