GroupBy uid shouldn't panic, and other logic fixes. (#2952)

* added check for nil destuid in child sg. also fixed a couple of logic bugs.

* fixed another logic bug related to algo.IndexOf

* fixed logic bug, made algo.ApplyFilter clearer, and simplified appendDummyValues a bit.

* added tests

* change algo.IndexOf to less explicit form

query/groupby.go

@@ -214,7 +214,7 @@ 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]
@@ -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
 		}
 	}
 

query/query.go

@@ -1379,8 +1379,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) >= 0 // Binary search.
 			})
 		} else {
 			// If we didn't order on UIDmatrix, it'll be sorted.
@@ -1772,12 +1771,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)

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 <name> "horsejr" .
+      _:x2 <name> "srfrog" .
+      _:x3 <name> "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))
+	}
+}