Add support for sorting on multiple facets

[ashish-goswami]

Fixes: #3638

Currently, we can specify ordering only on single facet while retrieving facets on uid predicates.
This PR adds support for specifying ordering on more than one facets.

---

This change is 

[animesh2049]

Logic seems correct to me. Just check if the comments about alternate implementation makes sense.

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pawanrawal)

---

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

			var facetsVal []types.Val

			for _, it := range f.Facets {

Instead of iterating over facetList, can we iterate over keys of facetsmap and do a binary search over facetList to find the facet?

---

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

					continue
				}
				if _, ok := facetsMap[it.Key]; !ok {

Can we have a test for the scenario when same facet is specified more than once for the same edge?

---

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

				}
			}
			values = append(values, facetsVal)

We can probably pre-allocate values matrix, use orderByKeys map to store the indices of facets in FacetsOrder and put the facet values at values[j][facetIndex] = val.
This way we don't need the loop above.

---

types/sort.go, line 95 at r1 (raw file):

	// TODO(Ashish): can we move this check to place where v is formed??
	// TODO(Ashish): Here we are checking type for values in v[0] only. This might not return

Good point. This would be solved if we check sortability while filling values in v

[pawanrawal]

Well written, easy to read change. Please address comments and feel free to merge.

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @animesh2049, @ashish-goswami, and @manishrjain)

---

gql/parser.go, line 115 at r2 (raw file):

type FacetOrder struct {
	Key       string
	OrderDesc bool

Desc

---

gql/parser.go, line 2021 at r2 (raw file):

	}

	facetsOrderKeys := make(map[string]struct{})

@facets(orderasc: key, alias: key)

Check what happens in this case.

---

gql/parser_test.go, line 3414 at r2 (raw file):

}

func TestParseOrderbyMultipleFacets(t *testing.T) {

Also add a case with multiple order, alias and requesting a couple of facet keys.

---

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Instead of iterating over facetList, can we iterate over keys of facetsmap and do a binary search over facetList to find the facet?

Add this as a TODO that we can do this optimization later in case facets are sorted by key.

---

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

			// Since any facet can come only once in f.Facets, we can have counter
			// to check if we have populated all facets or not.

Please add this to the comment as well.

Once we have found values for all facets that we are ordering by, we don't need to iterate any further and can break the loop.

---

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

					return err
				}
				values[j][idx] = fVal

You can return an error from from here if fVal.Tid is not sortable.

---

query/query_facets_test.go, line 349 at r2 (raw file):

}

func TestFacetsMultipleOrderby(t *testing.T) {

Please add a test which starts with two uids.

---

types/sort.go, line 94 at r2 (raw file):

	}

	// TODO(Ashish): can we move this check to place where v is formed??

Please check that if same facet has different type (string and int) across different uids, an error should be returned that we can't sort.

---

types/sort.go, line 98 at r2 (raw file):

	// correct result as v[0] might have DefaultID type for some val while some v[x]
	// can have some nonsortable type.
	for _, val := range v[0] {

Add a test case where we sort by multiple predicates and the second predicate is not sortable(bool).

[ashish-goswami]

Reviewable status: 1 of 6 files reviewed, 11 unresolved discussions (waiting on @animesh2049, @manishrjain, and @pawanrawal)

---

gql/parser.go, line 115 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Desc

Done.

---

gql/parser.go, line 2021 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

@facets(orderasc: key, alias: key)

Check what happens in this case.

It works fine. Parser takes care of this case by doing sorting on all Facets and removing duplicate.

---

gql/parser_test.go, line 3414 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Also add a case with multiple order, alias and requesting a couple of facet keys.

Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

Add this as a TODO that we can do this optimization later in case facets are sorted by key.

Done.

---

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Can we have a test for the scenario when same facet is specified more than once for the same edge?

I checked it, we cannot have repeated keys in facets. This fails in mutation itself.

---

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

We can probably pre-allocate values matrix, use orderByKeys map to store the indices of facets in FacetsOrder and put the facet values at values[j][facetIndex] = val.
This way we don't need the loop above.

Nice suggestion. It simplified the code.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

Please add this to the comment as well.

Once we have found values for all facets that we are ordering by, we don't need to iterate any further and can break the loop.

Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

You can return an error from from here if fVal.Tid is not sortable.

Done.

---

query/query_facets_test.go, line 349 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Please add a test which starts with two uids.

Done.

---

types/sort.go, line 94 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Please check that if same facet has different type (string and int) across different uids, an error should be returned that we can't sort.

Checked this. Currently we are not returning any error. If we are comparing two mismatched type, we sort based on type id.

func mismatchedLess(a, b Val) bool {
	x.AssertTrue(a.Tid != b.Tid)
	if (a.Tid != IntID && a.Tid != FloatID) || (b.Tid != IntID && b.Tid != FloatID) {
		// Non-float/int are sorted arbitrarily by type.
		return a.Tid < b.Tid
	}

	// Floats and ints can be sorted together in a sensible way. The approach
	// here isn't 100% correct, and will be wrong when dealing with ints and
	// floats close to each other and greater in magnitude than 1<<53 (the
	// point at which consecutive floats are more than 1 apart).
	if a.Tid == FloatID {
		return a.Value.(float64) < float64(b.Value.(int64))
	}
	x.AssertTrue(b.Tid == FloatID)
	return float64(a.Value.(int64)) < b.Value.(float64)
}

---

types/sort.go, line 98 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a test case where we sort by multiple predicates and the second predicate is not sortable(bool).

Its already done.

[manishrjain]

Just one comment. Good to go.

Reviewed 1 of 6 files at r1, 5 of 5 files at r3.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @animesh2049, @ashish-goswami, and @pawanrawal)

---

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

				}
				if !types.IsSortable(fVal.Tid) {
					return errors.Errorf("Value of type: %s isn't sortable", fVal.Tid.Name())

Perhaps, we should just ignore this value? Set to nil or something. That way, the query can still run.

---

types/sort.go, line 91 at r3 (raw file):

	switch tid {
	case DateTimeID, IntID, FloatID, StringID, DefaultID:
		return true

default:
return false

[ashish-goswami]

Reviewable status: 3 of 6 files reviewed, 13 unresolved discussions (waiting on @animesh2049, @manishrjain, and @pawanrawal)

---

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

Previously, manishrjain (Manish R Jain) wrote…

Perhaps, we should just ignore this value? Set to nil or something. That way, the query can still run.

Changed the code to ignore the value if type is not sortable.

---

types/sort.go, line 91 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

default:
return false

Done.