From 4b28558fa710e291c172857f060768b5eac06107 Mon Sep 17 00:00:00 2001 From: srfrog Date: Tue, 29 Jan 2019 20:01:31 -0700 Subject: [PATCH 1/5] added check for nil destuid in child sg. also fixed a couple of logic bugs. --- query/groupby.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/query/groupby.go b/query/groupby.go index fb8e71aa8e8..1162d0af8af 100644 --- a/query/groupby.go +++ b/query/groupby.go @@ -214,12 +214,12 @@ func (sg *SubGraph) formResult(ul *pb.List) (*groupResults, error) { if attr == "" { attr = child.Attr } - if len(child.DestUIDs.Uids) != 0 { + if child.DestUIDs != nil && len(child.DestUIDs.Uids) != 0 { // It's a UID node. for i := 0; i < len(child.uidMatrix); i++ { srcUid := child.SrcUIDs.Uids[i] // Ignore uids which are not part of srcUid. - if algo.IndexOf(ul, srcUid) < 0 { + if algo.IndexOf(ul, srcUid) == -1 { continue } ul := child.uidMatrix[i] @@ -231,7 +231,7 @@ func (sg *SubGraph) formResult(ul *pb.List) (*groupResults, error) { // It's a value node. for i, v := range child.valueMatrix { srcUid := child.SrcUIDs.Uids[i] - if len(v.Values) == 0 || algo.IndexOf(ul, srcUid) < 0 { + if len(v.Values) == 0 || algo.IndexOf(ul, srcUid) == -1 { continue } val, err := convertTo(v.Values[0]) @@ -271,10 +271,11 @@ func (sg *SubGraph) formResult(ul *pb.List) (*groupResults, error) { // that it considers the whole uidMatrix to do the grouping before assigning the variable. // TODO - Check if we can reduce this duplication. func (sg *SubGraph) fillGroupedVars(doneVars map[string]varValue, path []*SubGraph) error { - childHasVar := false + var childHasVar bool for _, child := range sg.Children { if child.Params.Var != "" { childHasVar = true + break } } From 7fd9a03998b7d47698432e71b58329bde1b6a556 Mon Sep 17 00:00:00 2001 From: srfrog Date: Tue, 29 Jan 2019 20:02:37 -0700 Subject: [PATCH 2/5] fixed another logic bug related to algo.IndexOf --- query/outputnode.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query/outputnode.go b/query/outputnode.go index d94bcb6db07..98d01acaee2 100644 --- a/query/outputnode.go +++ b/query/outputnode.go @@ -445,7 +445,7 @@ func processNodeUids(fj *fastJsonNode, sg *SubGraph) error { lenList := len(sg.uidMatrix[0].Uids) for i := 0; i < lenList; i++ { uid := sg.uidMatrix[0].Uids[i] - if algo.IndexOf(sg.DestUIDs, uid) < 0 { + if algo.IndexOf(sg.DestUIDs, uid) == -1 { // This UID was filtered. So Ignore it. continue } From dba4171ed19d14cff42262689a0d5a93a90c55d7 Mon Sep 17 00:00:00 2001 From: srfrog Date: Tue, 29 Jan 2019 20:03:08 -0700 Subject: [PATCH 3/5] fixed logic bug, made algo.ApplyFilter clearer, and simplified appendDummyValues a bit. --- query/query.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/query/query.go b/query/query.go index 096da94fb13..186857478df 100644 --- a/query/query.go +++ b/query/query.go @@ -416,7 +416,7 @@ func (sg *SubGraph) preTraverse(uid uint64, dst outputNode) error { } idx := algo.IndexOf(pc.SrcUIDs, uid) - if idx < 0 { + if idx == -1 { continue } if pc.Params.isGroupBy { @@ -1371,8 +1371,7 @@ func (sg *SubGraph) updateUidMatrix() { // We can't do intersection directly as the list is not sorted by UIDs. // So do filter. algo.ApplyFilter(l, func(uid uint64, idx int) bool { - i := algo.IndexOf(sg.DestUIDs, uid) // Binary search. - return i >= 0 + return algo.IndexOf(sg.DestUIDs, uid) != -1 // Binary search. }) } else { // If we didn't order on UIDmatrix, it'll be sorted. @@ -1753,12 +1752,12 @@ func (sg *SubGraph) ApplyIneqFunc() error { } func (sg *SubGraph) appendDummyValues() { - if sg.SrcUIDs == nil { + if sg.SrcUIDs == nil || len(sg.SrcUIDs.Uids) == 0 { return } var l pb.List var val pb.ValueList - for i := 0; i < len(sg.SrcUIDs.Uids); i++ { + for range sg.SrcUIDs.Uids { // This is necessary so that preTraverse can be processed smoothly. sg.uidMatrix = append(sg.uidMatrix, &l) sg.valueMatrix = append(sg.valueMatrix, &val) From f3231f49d25b4af67c091ed96a5a99b204cb7625 Mon Sep 17 00:00:00 2001 From: srfrog Date: Tue, 29 Jan 2019 21:13:06 -0700 Subject: [PATCH 4/5] added tests --- systest/queries_test.go | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/systest/queries_test.go b/systest/queries_test.go index 069dca6bc32..c884963f8ed 100644 --- a/systest/queries_test.go +++ b/systest/queries_test.go @@ -51,6 +51,7 @@ func TestQuery(t *testing.T) { t.Run("unmatched var assignment eval", wrap(UnmatchedVarEval)) t.Run("hash index queries", wrap(QueryHashIndex)) t.Run("regexp with toggled trigram index", wrap(RegexpToggleTrigramIndex)) + t.Run("groupby uid that works", wrap(GroupByUidWorks)) t.Run("cleanup", wrap(SchemaQueryCleanup)) } @@ -723,3 +724,48 @@ func RegexpToggleTrigramIndex(t *testing.T, c *dgo.Dgraph) { require.Error(t, err) require.Contains(t, err.Error(), "Attribute name does not have trigram index for regex matching.") } + +func GroupByUidWorks(t *testing.T, c *dgo.Dgraph) { + ctx := context.Background() + + txn := c.NewTxn() + assigned, err := txn.Mutate(ctx, &api.Mutation{ + SetNquads: []byte(` + _:x1 "horsejr" . + _:x2 "srfrog" . + _:x3 "missbug" . + `), + }) + require.NoError(t, err) + require.NoError(t, txn.Commit(ctx)) + + tests := []struct { + in, out string + }{ + { + in: fmt.Sprintf(`{q(func:uid(%s)) @groupby(uid) {count(uid)}}`, assigned.Uids["x1"]), + out: `{}`, + }, + { + in: fmt.Sprintf(`{q(func:uid(%s)) @groupby(name) {count(uid)}}`, assigned.Uids["x1"]), + out: `{"q":[{ + "@groupby":[{ + "count": 1, + "name": "horsejr" + }]}]}`, + }, + { + in: `{q(func:has(dummy)) @groupby(uid) {}}`, + out: `{}`, + }, + { + in: `{q(func:has(name)) @groupby(dummy) {}}`, + out: `{}`, + }, + } + for _, tc := range tests { + resp, err := c.NewTxn().Query(ctx, tc.in) + require.NoError(t, err) + CompareJSON(t, tc.out, string(resp.Json)) + } +} From 17751c232e1c7ebed17c86a63c42dadeab38ae7c Mon Sep 17 00:00:00 2001 From: srfrog Date: Wed, 30 Jan 2019 20:41:19 -0700 Subject: [PATCH 5/5] change algo.IndexOf to less explicit form --- query/groupby.go | 4 ++-- query/outputnode.go | 2 +- query/query.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/query/groupby.go b/query/groupby.go index 1162d0af8af..726aee63ea5 100644 --- a/query/groupby.go +++ b/query/groupby.go @@ -219,7 +219,7 @@ func (sg *SubGraph) formResult(ul *pb.List) (*groupResults, error) { for i := 0; i < len(child.uidMatrix); i++ { srcUid := child.SrcUIDs.Uids[i] // Ignore uids which are not part of srcUid. - if algo.IndexOf(ul, srcUid) == -1 { + if algo.IndexOf(ul, srcUid) < 0 { continue } ul := child.uidMatrix[i] @@ -231,7 +231,7 @@ func (sg *SubGraph) formResult(ul *pb.List) (*groupResults, error) { // It's a value node. for i, v := range child.valueMatrix { srcUid := child.SrcUIDs.Uids[i] - if len(v.Values) == 0 || algo.IndexOf(ul, srcUid) == -1 { + if len(v.Values) == 0 || algo.IndexOf(ul, srcUid) < 0 { continue } val, err := convertTo(v.Values[0]) diff --git a/query/outputnode.go b/query/outputnode.go index 98d01acaee2..d94bcb6db07 100644 --- a/query/outputnode.go +++ b/query/outputnode.go @@ -445,7 +445,7 @@ func processNodeUids(fj *fastJsonNode, sg *SubGraph) error { lenList := len(sg.uidMatrix[0].Uids) for i := 0; i < lenList; i++ { uid := sg.uidMatrix[0].Uids[i] - if algo.IndexOf(sg.DestUIDs, uid) == -1 { + if algo.IndexOf(sg.DestUIDs, uid) < 0 { // This UID was filtered. So Ignore it. continue } diff --git a/query/query.go b/query/query.go index 186857478df..5aca1915f1c 100644 --- a/query/query.go +++ b/query/query.go @@ -416,7 +416,7 @@ func (sg *SubGraph) preTraverse(uid uint64, dst outputNode) error { } idx := algo.IndexOf(pc.SrcUIDs, uid) - if idx == -1 { + if idx < 0 { continue } if pc.Params.isGroupBy { @@ -1371,7 +1371,7 @@ func (sg *SubGraph) updateUidMatrix() { // We can't do intersection directly as the list is not sorted by UIDs. // So do filter. algo.ApplyFilter(l, func(uid uint64, idx int) bool { - return algo.IndexOf(sg.DestUIDs, uid) != -1 // Binary search. + return algo.IndexOf(sg.DestUIDs, uid) >= 0 // Binary search. }) } else { // If we didn't order on UIDmatrix, it'll be sorted.