Support for count(uid) with alias #4122
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.
| 1 Message | |
|---|---|
| ✅ |
|
mangalaman93
force-pushed
the
aman/issue_4038
branch
from
fd3307e to
8492c45
Compare
There was a problem hiding this comment.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
There was a problem hiding this comment.
Less is more. Good job reducing the amount of fields/bloat and maintaining the inherent functionality.
Reviewed with ❤️ by PullRequest
mangalaman93
force-pushed
the
aman/issue_4038
branch
2 times, most recently
from
6cb796c to
41b9684
Compare
mangalaman93
requested review from
manishrjain,
pawanrawal and
a team
as code owners
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])
gql/parser.go, line 2843 at r2 (raw file):
Previously, pullrequest[bot] wrote…
I see this is being used as a placeholder for a blank alias. Does this field need to be set at all then? If it's required, can it be made optional?
Done.
query/outputnode.go, line 613 at r2 (raw file):
Previously, pullrequest[bot] wrote…
Do we want to look at DestUIDs here?
Done.
query/outputnode.go, line 615 at r2 (raw file):
Previously, pullrequest[bot] wrote…
Isn't
UidCountAliasdeprecated? I don't see another instance of this in the repo.
yes.
query/query.go, line 1135 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)math.MaxUint64: {
Done.
query/query0_test.go, line 2289 at r2 (raw file):
Previously, pullrequest[bot] wrote…
TODO comments are a better way of codepaths that need to be worked on rather than commenting out code. Git will keep track of the latter for you. The former will explain why something is missing.
Done.
mangalaman93
force-pushed
the
aman/issue_4038
branch
from
41b9684 to
458fd7a
Compare
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
query/query4_test.go, line 975 at r3 (raw file):
}` _, err := processQuery(context.Background(), t, query) require.Contains(t, err.Error(), "uidcount not allowed multiple times in same sub-query")
Should the error be count(uid) not allowed ...?
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
query/query4_test.go, line 975 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Should the error be
count(uid) not allowed ...?
I can add one more if to handle this case, but I feel this is also explanatory of the issue.
There was a problem hiding this comment.
Looks nicely written from a cursory look. Have one comment. LGTM from my side. Feel free to merge once you get approval from @pawanrawal .
Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
query/query4_test.go, line 1085 at r3 (raw file):
"q": [ { "count": 3
What happens if I have count(uid) and count(friend) on the same level? What would the result contain as the JSON key?
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1, 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @golangcibot, @mangalaman93, @pawanrawal, and @pullrequest[bot])
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
query/query4_test.go, line 1085 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
What happens if I have count(uid) and count(friend) on the same level? What would the result contain as the JSON key?
count(friend) will be handled at the child level whereas count(uid) nodes are handled at the parent. For example -
{
q(func: has(friend)) {
name
count(uid)
count(friend)
}
}
{
"data": {
"q": [
{
"count": 3
},
{
"name": "Michonne",
"count(friend)": 5
},
{
"name": "Rick Grimes",
"count(friend)": 1
},
{
"name": "Andrea",
"count(friend)": 1
}
]
},
"extensions": {
"server_latency": {
"parsing_ns": 11096,
"processing_ns": 13086401,
"encoding_ns": 22698,
"assign_timestamp_ns": 566487
},
"txn": {
"start_ts": 121
}
}
}
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])
This change is