Support/groupby

[ashwin95r]

This change is 

[manishrjain]

Review status: 0 of 5 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

query/groupby.go, line 35 at r2 (raw file):

}

type groups struct {

Move this below GroupInfo. groupResults

---

query/groupby.go, line 39 at r2 (raw file):

}

type GroupInfo struct {

make it private. groupResult

---

query/groupby.go, line 40 at r2 (raw file):

type GroupInfo struct {
	values     []groupPair

keys []groupPair

---

query/groupby.go, line 47 at r2 (raw file):

type groupElements struct {
	entities *taskp.List
	val      types.Val

key types.Val

---

query/groupby.go, line 51 at r2 (raw file):

type dedup struct {
	mp    []map[string]groupElements

Move mp and attrs out to a new struct. So, dedup would only have a list of that struct.

type S struct {
  mp map[string]groupElements
  attr string
}

---

query/groupby.go, line 191 at r2 (raw file):

	// Go over the groups and aggregate the values.
	for i := range res.group {
		grp := &res.group[i]

grp := res.group[i]. So, you don't need (*grp).

---

query/groupby.go, line 218 at r2 (raw file):

		}
	}
	sort.Slice(res.group, func(i, j int) bool {

Remove?

---

query/query.go, line 999 at r1 (raw file):

	mp := make(map[string]GroupInfo)
	_ = mp
	dedupMap := make([]map[string][]idVal, len(sg.Params.groupbyAttrs))

Make this a struct, with functions to aid what you're trying to achieve here.

attrList can be done via this struct.

---

Comments from Reviewable

[ashwin95r]

Review status: 0 of 5 files reviewed at latest revision, 8 unresolved discussions.

---

query/groupby.go, line 35 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move this below GroupInfo. groupResults

Done.

---

query/groupby.go, line 39 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

make it private. groupResult

Done.

---

query/groupby.go, line 40 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

keys []groupPair

Done.

---

query/groupby.go, line 47 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

key types.Val

Done.

---

query/groupby.go, line 51 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move mp and attrs out to a new struct. So, dedup would only have a list of that struct.

type S struct {
  mp map[string]groupElements
  attr string
}

Done.

---

query/groupby.go, line 191 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

grp := res.group[i]. So, you don't need (*grp).

Done.

---

query/groupby.go, line 218 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove?

Done.

---

query/query.go, line 999 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make this a struct, with functions to aid what you're trying to achieve here.

attrList can be done via this struct.

Done.

---

Comments from Reviewable

[manishrjain]

Fix the comments, and wait for @janardhan1993 to have a look as well.

---

Reviewed 2 of 5 files at r1, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

query/groupby.go, line 44 at r3 (raw file):

func (grp *groupResult) aggregateChild(child *SubGraph) {
	if child.Params.DoCount {
		(*grp).aggregates = append((*grp).aggregates, groupPair{

grp.aggregates? here and elsewhere.

---

query/groupby.go, line 50 at r3 (raw file):

				Value: int64(len(grp.uids)),
			},
		})

return

---

query/groupby.go, line 51 at r3 (raw file):

			},
		})
	} else if len(child.SrcFunc) > 0 && isAggregatorFn(child.SrcFunc[0]) {

if

---

query/groupby.go, line 85 at r3 (raw file):

	idx := -1
	// Going last to first as we'll always add new ones to last and
	// would access it till we add a new entry.

This could be reworded.

---

query/groupby.go, line 100 at r3 (raw file):

		})
		idx = len(d.groups) - 1
	}

Refactor this part in a separate function, which would return uniq instance.

---

query/groupby.go, line 122 at r3 (raw file):

		}
	}
	cur := d.groups[idx].elements[strKey].entities

You don't need the idx, just use the uniq instance from above.

---

query/groupby.go, line 125 at r3 (raw file):

	cur.Uids = append(cur.Uids, uid)
}

Add a comment.

---

query/groupby.go, line 233 at r3 (raw file):

}

func groupLess(a, b *groupResult) bool {

Let's remove any group which has zero results proactively.

---

Comments from Reviewable

[janardhan1993]

---

Reviewed 2 of 5 files at r1, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

gql/parser.go, line 1462 at r3 (raw file):

		} else if item.Typ == itemName {
			if !expectArg {
				return x.Errorf("Expected a variable but got comma")

expected a comma or right round ?

---

gql/parser.go, line 1472 at r3 (raw file):

	}
	if count == 0 {
		return x.Errorf("Expected atleast one attribute in groupby")

Count is never incremented, so it would be always zero ?

---

query/groupby.go, line 52 at r3 (raw file):

		})
	} else if len(child.SrcFunc) > 0 && isAggregatorFn(child.SrcFunc[0]) {
		fieldName := fmt.Sprintf("%s(%s)", child.SrcFunc[0], child.Attr)

Off the topic, in normal query(without group by) fieldname would be same for both ? Wouldn't that give wrong results if we have both in the query
count(abc)
count(abc @filter())

---

Comments from Reviewable

[janardhan1993]

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

query/query_test.go, line 933 at r3 (raw file):

		{
			me(id: 1) {
				friend @groupby(friend,name) {

Does it make sense to group by friend?

---

query/query_test.go, line 941 at r3 (raw file):

	js := processToFastJSON(t, query)
	require.JSONEq(t,
		`{"me":[{"friend":{"@groupby":[{"count":0,"friend":"0x1","name":"Andrea"},{"count":0,"friend":"0x1","name":"Daryl Dixon"},{"count":0,"friend":"0x1","name":"Glenn Rhee"},{"count":0,"friend":"0x18","name":"Daryl Dixon"},{"count":0,"friend":"0x18","name":"Glenn Rhee"},{"count":0,"friend":"0x18","name":"Rick Grimes"},{"count":1,"friend":"0x1","name":"Rick Grimes"},{"count":1,"friend":"0x18","name":"Andrea"}]}}]}`,

There are 5 friends in total, but the output is showing only 2 ?(2 friends * 4 names)

---

Comments from Reviewable

[janardhan1993]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

query/query_test.go, line 917 at r3 (raw file):

			me(id: 1) {
				friend @groupby(age) {
					max(name)

Add a test case with age stored as both int and float may be

---

Comments from Reviewable

[ashwin95r]

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

gql/parser.go, line 1462 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

expected a comma or right round ?

Done.

---

gql/parser.go, line 1472 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Count is never incremented, so it would be always zero ?

Done.

---

query/groupby.go, line 44 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

grp.aggregates? here and elsewhere.

Done.

---

query/groupby.go, line 50 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return

Done.

---

query/groupby.go, line 51 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if

Done.

---

query/groupby.go, line 52 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Off the topic, in normal query(without group by) fieldname would be same for both ? Wouldn't that give wrong results if we have both in the query
count(abc)
count(abc @filter())

count of the same attribute isn't allowed twice.

---

query/groupby.go, line 85 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This could be reworded.

Done.

---

query/groupby.go, line 100 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Refactor this part in a separate function, which would return uniq instance.

Done.

---

query/groupby.go, line 122 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You don't need the idx, just use the uniq instance from above.

Done.

---

query/groupby.go, line 125 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a comment.

Done.

---

query/groupby.go, line 233 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Let's remove any group which has zero results proactively.

Done.

---

query/query_test.go, line 917 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Add a test case with age stored as both int and float may be

This operates on the value age which would be converted to schema type before groupby is applied. So it shouldn't affect the working.

---

query/query_test.go, line 933 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Does it make sense to group by friend?

Just for testing, wouldn't make much sense in reality.

---

query/query_test.go, line 941 at r3 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

There are 5 friends in total, but the output is showing only 2 ?(2 friends * 4 names)

yeah, Its friend of the first level friend. Groupby operates on the child predicates of the current level.

---

Comments from Reviewable

[ashwin95r]

Merged to master.