Support @normalize directive for subqueries #4042
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.
@animesh2049 you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @animesh2049)
query/outputnode.go, line 64 at r1 (raw file):
New(attr string) outputNode SetUID(uid uint64, attr string) SetFlatten(flatten bool)
Can we call it SetNormalize instead? or something equivalent. Doesn't make sense to use two different names for the same thing
query/outputnode.go, line 815 at r1 (raw file):
} func createAttrs(oNode outputNode) error {
This function is not needed, implementation can be moved in the caller itself
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @golangcibot and @mangalaman93)
query/outputnode.go, line 64 at r1 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
Can we call it SetNormalize instead? or something equivalent. Doesn't make sense to use two different names for the same thing
Done.
query/outputnode.go, line 815 at r1 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
This function is not needed, implementation can be moved in the caller itself
Done.
query/outputnode.go, line 841 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
Error return value of
flattenResultis not checked (fromerrcheck)
Done.
There was a problem hiding this comment.
I've left a single inline comment regarding struct initialization and accessing its pointer value.
Reviewed with ❤️ by PullRequest
query/outputnode.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not a blocker but a readability recommendation, prefer being explicit that 2 operations are happening.
jn := fastJsonNode{attr: node.attr, attrs: attrList[0]}
parent.attrs[childIdx] = &jn
query/outputnode.go
Outdated
| normalized, err := n1.(*fastJsonNode).normalize() | ||
| if err != nil { | ||
| fj.AddListChild(sg.Params.Alias, n1) | ||
| if err := flattenResult(n1.(*fastJsonNode), fj, len(fj.attrs)-1); err != nil { |
There was a problem hiding this comment.
Is it possible that fj.attrs returns 0? Is the resulting -1 a valid argument to flattenResult?
animesh2049
force-pushed
the
animesh2049/normalize_subqueries
branch
from
9dd6cda
to
f390c8b
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.
Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @animesh2049 and @golangcibot)
query/outputnode.go, line 482 at r3 (raw file):
} if err := normalizeResult(fj, nil, 0); err != nil {
Why are we passing 0 here? add a comment
query/outputnode.go, line 828 at r3 (raw file):
for _, attrs := range attrList[1:] { parent.attrs = append(parent.attrs, &fastJsonNode{attr: node.attr, attrs: attrs})
isChild copy?
query/query2_test.go, line 1559 at r3 (raw file):
func TestNormalizeDirectiveSubQueryLevel1(t *testing.T) {
newline
query/query2_test.go, line 1628 at r3 (raw file):
js := processQueryNoErr(t, query) require.JSONEq(t, `{"data":{"me":[{"fn":"Michonne","mn":"Michonne","n":"Rick Grimes","sn":"Andre"},{"fn":"Michonne","mn":"Michonne","n":"Rick Grimes","sn":"Helmut"},{"mn":"Michonne","n":"Glenn Rhee","sn":"Andre"},{"mn":"Michonne","n":"Glenn Rhee","sn":"Helmut"},{"mn":"Michonne","n":"Daryl Dixon","sn":"Andre"},{"mn":"Michonne","n":"Daryl Dixon","sn":"Helmut"},{"fn":"Glenn Rhee","mn":"Michonne","n":"Andrea","sn":"Andre"},{"fn":"Glenn Rhee","mn":"Michonne","n":"Andrea","sn":"Helmut"}]}}`, js)
format
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @animesh2049, @golangcibot, @mangalaman93, and @pullrequest[bot])
query/outputnode.go, line 480 at r2 (raw file):
Previously, pullrequest[bot] wrote…
Is it possible that
fj.attrsreturns 0? Is the resulting-1a valid argument toflattenResult?
Done.
query/outputnode.go, line 482 at r3 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
Why are we passing 0 here? add a comment
Done.
query/outputnode.go, line 828 at r3 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
isChild copy?
Done.
query/query2_test.go, line 1559 at r3 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
newline
Done.
query/query2_test.go, line 1628 at r3 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
format
Done.
animesh2049
requested review from
danielmai,
manishrjain,
MichaelJCompton,
pawanrawal and
a team
as code owners
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
a discussion (no related file):
query/outputnode.go, line 834 at r4 (raw file):
for _, attrs := range attrList[1:] { parent.attrs = append(parent.attrs, &fastJsonNode{attr: node.attr, attrs: attrs, isChild: node.isChild})
nit: may be define the node first into a variable and then perform append
There was a problem hiding this comment.
Mostly looks good. I'd still want you to modify the tests to start with multiple ids at root and please see other comments as well. I'll have another look after you have replied to them.
Reviewable status: 0 of 6 files reviewed, 13 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode.go, line 64 at r4 (raw file):
New(attr string) outputNode SetUID(uid uint64, attr string) SetNormalize(isNormalized bool)
Do you need to pass in parameter here? Or could you just set the value to true whenever this function is called on fastJsonNode.
query/outputnode.go, line 88 at r4 (raw file):
attrs []*fastJsonNode list bool isNormalized bool
Please add a comment about when is this set to true. For e.g. is it only set for a node on which the directive was applied or for all its children as well?
query/outputnode.go, line 829 at r4 (raw file):
parent.attrs[childIdx] = &fastJsonNode{ // A small optimization. Instead of removing attr: node.attr, // element from slice, we are replacing it attrs: attrList[0], // with one of the node that we intend to
Please move these comments to above the code. Inline comments are ok if they are only on a single line.
query/outputnode.go, line 836 at r4 (raw file):
&fastJsonNode{attr: node.attr, attrs: attrs, isChild: node.isChild}) } } else {
You can return before this line and then you don't need else can out indent the block below.
query/query1_test.go, line 1067 at r4 (raw file):
}` js := processQueryNoErr(t, query) require.JSONEq(t, `{"data":{"me":[{"Friend":"Rick Grimes","Me":"Michonne"},{"Cofriend":"Glenn Rhee","Friend":"Michonne","Me":"Rick Grimes"},{"Me":"Daryl Dixon"},{"Friend":"Glenn Rhee","Me":"Michonne"},{"Friend":"Daryl Dixon","Me":"Michonne"},{"Cofriend":"Glenn Rhee","Friend":"Andrea","Me":"Michonne"},{"Cofriend":"Daryl Dixon","Friend":"Michonne","Me":"Rick Grimes"},{"Cofriend":"Andrea","Friend":"Michonne","Me":"Rick Grimes"}]}}`, js)
Why did the response change here? Was the previous result wrong?
query/query2_test.go, line 1616 at r4 (raw file):
query := ` { me(func: uid(0x01)) {
Ideally, I'd prefer we start with multiple uids always in tests. Or if you want you can have tests both for single and multiple ids. In the past I have encountered bugs where things worked with single ids but not multiple.
query/query2_test.go, line 1681 at r4 (raw file):
dob friend @normalize { # Results of this subquery will be normalized fn : name
Can you add another field without an alias to this test and also possibly another level of nesting here? Right now the result of the query seems to be same as what it would be without the directive.
query/query2_test.go, line 1740 at r4 (raw file):
}`, js) }
Can you please add one more test where two sibling nodes with children have @normalize? Something like the example below, fill in values for some_predicate and its children.
{
me(func: uid(0x01, 0x02, 0x03)) {
mn: name
gender
friend @normalize {
n: name
dob
friend {
fn : name
}
}
some_predicate @normalize {
child {
n: name
}
}
}
}
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 13 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode.go, line 64 at r4 (raw file):
dst.SetNormalize(sg.Params.Normalize)
I will have to check if sg.Params.Normalize is true and then call SetNormalize. Will that be better ?
query/outputnode.go, line 88 at r4 (raw file):
It will be true for children as well.
Normalize: gchild.Normalize || sg.Params.Normalize,
I am recursively setting Normalized to true for all the children of node where @normalize has been applied. Later in pretraverse setting isNormalized when Normalized is true.
I also added a comment.
query/outputnode.go, line 829 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Please move these comments to above the code. Inline comments are ok if they are only on a single line.
Done.
query/outputnode.go, line 836 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
You can return before this line and then you don't need else can out indent the block below.
Done.
query/query1_test.go, line 1067 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Why did the response change here? Was the previous result wrong?
Only the order of output in the list has changed. Test was failing because of that.
query/query2_test.go, line 1616 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Ideally, I'd prefer we start with multiple uids always in tests. Or if you want you can have tests both for single and multiple ids. In the past I have encountered bugs where things worked with single ids but not multiple.
Done.
query/query2_test.go, line 1681 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can you add another field without an alias to this test and also possibly another level of nesting here? Right now the result of the query seems to be same as what it would be without the directive.
Done
query/query2_test.go, line 1740 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can you please add one more test where two sibling nodes with children have
@normalize? Something like the example below, fill in values forsome_predicateand its children.{ me(func: uid(0x01, 0x02, 0x03)) { mn: name gender friend @normalize { n: name dob friend { fn : name } } some_predicate @normalize { child { n: name } } } }
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 14 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
wiki/content/query-language/index.md, line 1899 at r5 (raw file):
{{< /runnable >}} You can also apply `@normalize` on nested query blocks. It will work similarly but only flatten the result of nested query block where `@normalize` has been applied.
the result of the nested query block ...
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, and @pullrequest[bot])
a discussion (no related file):
Previously, mangalaman93 (Aman Mangal) wrote…
:lgtm:
query/outputnode.go, line 64 at r4 (raw file):
Previously, animesh2049 (Animesh Chandra Pathak) wrote…
dst.SetNormalize(sg.Params.Normalize)
I will have to check if
sg.Params.Normalizeis true and then callSetNormalize. Will that be better ?
that's right. No this is ok in that case.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @animesh2049, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, and @pullrequest[bot])
a discussion (no related file):
Previously, pawanrawal (Pawan Rawal) wrote…
:lgtm:
Is this doing more work than before? Because of the extra recursive iteration this is doing?
If this PR isn't affecting performance, then
query/query2_test.go, line 1767 at r5 (raw file):
n: name dob friend @normalize { # It would appear as if @normalize isn't here.
"This would be ignored."
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, and @pullrequest[bot])
query/query2_test.go, line 1767 at r5 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
"This would be ignored."
Done.
wiki/content/query-language/index.md, line 1899 at r5 (raw file):
Previously, campoy (Francesc Campoy) wrote…
the result of the nested query block ...
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, and @pullrequest[bot])
a discussion (no related file):
Previously, manishrjain (Manish R Jain) wrote…
Is this doing more work than before? Because of the extra recursive iteration this is doing?
If this PR isn't affecting performance, then :lgtm:
Looks much simpler now.
Can you also check with a query which has an intermediate node of type uid and not [uid]? Also are you going to check in the benchmarks that you did before and after this change?
query/outputnode_test.go, line 98 at r6 (raw file):
} func TestNormalizeJSONUid1(t *testing.T) {
Why remove these?
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode_test.go, line 98 at r6 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Why remove these?
I have changed normalize function. Now instead of recursing it just flattens it's children. It expects all it's children have already called normalize flattened their children.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
a discussion (no related file):
Previously, pawanrawal (Pawan Rawal) wrote…
Looks much simpler now.
Can you also check with a query which has an intermediate node of type
uidand not[uid]? Also are you going to check in the benchmarks that you did before and after this change?
Benchmark that I wrote earlier are no more valid because of the recent changes. However I tried running normalize query from docs on 21million dataset. I don't see significant latency changes.
When creating new fastJSONNodes set isChild parameters. Also change the previous output of TestNormalizeSubqueryLevel2
animesh2049
force-pushed
the
animesh2049/normalize_subqueries
branch
from
c148603
to
7b623d8
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 13 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode.go, line 681 at r9 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Please add a comment that addAsList is only used for Normalize queries and not for others.
Removed it and added everything as list when for @normalize
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 20 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode.go, line 302 at r10 (raw file):
cnt := 0 for _, a := range fj.attrs { // Here we are counting all non scalar attributes of fj. If there are any such
all non-scalar attributes in fj.attrs
query/outputnode.go, line 304 at r10 (raw file):
// Here we are counting all non scalar attributes of fj. If there are any such // attributes, we will flatten it, otherwise we will return all attributes.
what is it here? fj?
query/outputnode.go, line 305 at r10 (raw file):
// attributes, we will flatten it, otherwise we will return all attributes. // When we call addMapChild it tries to find whether there is already a attribute
already an attribute
query/outputnode.go, line 309 at r10 (raw file):
// such attribute, it creates an attribute with isChild = false. In those cases // sometimes cnt remains zero and normalize returns attributes without flattening. // So we are using len(a.attrs) > 0 instead of a.isChild
Try to write this again in a much more clear way.
Think about someone that doesn't understand your code yet and how this would help them.
query/outputnode.go, line 327 at r10 (raw file):
for _, a := range fj.attrs { // Check comment at previous occurrence of len(a.attrs) > 0 if len(a.attrs) == 0 {
I would instead write a quick function, document it, and use it in all these places.
It will be inlined anyway and it will make your code more obvious.
query/outputnode.go, line 723 at r10 (raw file):
// calling normalize after pretraverse. // Now normalize() only flattens one level, // the expectation is that it's children have
its children
query/outputnode_test.go, line 98 at r10 (raw file):
} func TestNormalizeJSONUid1(t *testing.T) {
Why are you removing these tests?
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 20 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
query/outputnode.go, line 302 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
all non-scalar attributes in fj.attrs
Done.
query/outputnode.go, line 304 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
what is it here? fj?
Yes
query/outputnode.go, line 305 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
already an attribute
Done.
query/outputnode.go, line 309 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
Try to write this again in a much more clear way.
Think about someone that doesn't understand your code yet and how this would help them.
This is not exactly the explanation of what code is doing, rather I am trying to explain why I am using len(a.ttrs), so I guess reader must have some context about code.
query/outputnode.go, line 327 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
I would instead write a quick function, document it, and use it in all these places.
It will be inlined anyway and it will make your code more obvious.
I am using two different variations len(node.attrs)>0 and len(node.attrs) == 0. I will probably have to write 2 different functions. Would that be better ?
query/outputnode.go, line 723 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
its children
Done.
query/outputnode_test.go, line 98 at r10 (raw file):
Previously, campoy (Francesc Campoy) wrote…
Why are you removing these tests?
Behavior of normalize function has changed, so tests aren't valid anymore.
There was a problem hiding this comment.
Ok. Good to go. Assuming that we're not doing any more work than before, and the query latency is not going to be affected.
Reviewable status: 2 of 7 files reviewed, 20 unresolved discussions (waiting on @animesh2049, @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
There was a problem hiding this comment.
Dismissed @pawanrawal and @pullrequest[bot] from 2 discussions.
Reviewable status: 1 of 7 files reviewed, 18 unresolved discussions (waiting on @campoy, @danielmai, @golangcibot, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])
@normalizefor subqueries.resolves #1755
This change is