Add support for sorting on multiple facets

gql/parser.go

@@ -73,8 +73,7 @@ type GraphQuery struct {
 	FacetsFilter     *FilterTree
 	GroupbyAttrs     []GroupByAttr
 	FacetVar         map[string]string
-	FacetOrder       string
-	FacetDesc        bool
+	FacetsOrder      []*FacetOrder
 
 	// Internal fields below.
 	// If gq.fragment is nonempty, then it is a fragment reference / spread.
@@ -110,6 +109,12 @@ type GroupByAttr struct {
 	Langs []string
 }
 
+// FacetOrder stores ordering for single facet key.
+type FacetOrder struct {
+	Key  string
+	Desc bool // true if ordering should be decending by this facet.
+}
+
 // pair denotes the key value pair that is part of the GraphQL query root in parenthesis.
 type pair struct {
 	Key string
@@ -1902,11 +1907,10 @@ L:
 }
 
 type facetRes struct {
-	f          *pb.FacetParams
-	ft         *FilterTree
-	vmap       map[string]string
-	facetOrder string
-	orderdesc  bool
+	f           *pb.FacetParams
+	ft          *FilterTree
+	vmap        map[string]string
+	facetsOrder []*FacetOrder
 }
 
 func parseFacets(it *lex.ItemIterator) (res facetRes, err error) {
@@ -2006,15 +2010,15 @@ func tryParseFacetList(it *lex.ItemIterator) (res facetRes, parseOk bool, err er
 
 	facetVar := make(map[string]string)
 	var facets pb.FacetParams
-	var orderdesc bool
-	var orderkey string
+	var facetsOrder []*FacetOrder
 
 	if _, ok := tryParseItemType(it, itemRightRound); ok {
 		// @facets() just parses to an empty set of facets.
-		res.f, res.vmap, res.facetOrder, res.orderdesc = &facets, facetVar, orderkey, orderdesc
+		res.f, res.vmap, res.facetsOrder = &facets, facetVar, facetsOrder
 		return res, true, nil
 	}
 
+	facetsOrderKeys := make(map[string]struct{})
 	for {
 		// We've just consumed a leftRound or a comma.
 
@@ -2041,12 +2045,13 @@ func tryParseFacetList(it *lex.ItemIterator) (res facetRes, parseOk bool, err er
 				Alias: facetItem.alias,
 			})
 			if facetItem.ordered {
-				if orderkey != "" {
+				if _, ok := facetsOrderKeys[facetItem.name]; ok {
 					return res, false,
-						facetItemIt.Errorf("Invalid use of orderasc/orderdesc in facets")
+						it.Errorf("Sorting by facet: [%s] can only be done once", facetItem.name)
 				}
-				orderdesc = facetItem.orderdesc
-				orderkey = facetItem.name
+				facetsOrderKeys[facetItem.name] = struct{}{}
+				facetsOrder = append(facetsOrder,
+					&FacetOrder{Key: facetItem.name, Desc: facetItem.orderdesc})
 			}
 		}
 
@@ -2066,7 +2071,7 @@ func tryParseFacetList(it *lex.ItemIterator) (res facetRes, parseOk bool, err er
 			}
 			out = append(out, facets.Param[flen-1])
 			facets.Param = out
-			res.f, res.vmap, res.facetOrder, res.orderdesc = &facets, facetVar, orderkey, orderdesc
+			res.f, res.vmap, res.facetsOrder = &facets, facetVar, facetsOrder
 			return res, true, nil
 		}
 		if item, ok := tryParseItemType(it, itemComma); !ok {
@@ -2401,8 +2406,7 @@ func parseDirective(it *lex.ItemIterator, curp *GraphQuery) error {
 		switch {
 		case res.f != nil:
 			curp.FacetVar = res.vmap
-			curp.FacetOrder = res.facetOrder
-			curp.FacetDesc = res.orderdesc
+			curp.FacetsOrder = res.facetsOrder
 			if curp.Facets != nil {
 				return item.Errorf("Only one facets allowed")
 			}

gql/parser_test.go

@@ -3407,8 +3407,78 @@ func TestParseFacets(t *testing.T) {
 	require.Equal(t, []string{"friends"}, childAttrs(res.Query[0]))
 	require.NotNil(t, res.Query[0].Children[0].Facets)
 	require.Equal(t, []string{"name"}, childAttrs(res.Query[0].Children[0]))
-	require.Equal(t, "closeness", res.Query[0].Children[0].FacetOrder)
-	require.True(t, res.Query[0].Children[0].FacetDesc)
+	require.Equal(t, "closeness", res.Query[0].Children[0].FacetsOrder[0].Key)
+	require.True(t, res.Query[0].Children[0].FacetsOrder[0].Desc)
+}
+
+func TestParseOrderbyMultipleFacets(t *testing.T) {
+	query := `
+	query {
+		me(func: uid(0x1)) {
+			friends @facets(orderdesc: closeness, orderasc: since) {
+				name
+			}
+		}
+	}
+`
+	res, err := Parse(Request{Str: query})
+	require.NoError(t, err)
+	require.NotNil(t, res.Query[0])
+	require.Equal(t, []string{"friends"}, childAttrs(res.Query[0]))
+	require.NotNil(t, res.Query[0].Children[0].Facets)
+	require.Equal(t, []string{"name"}, childAttrs(res.Query[0].Children[0]))
+	require.Equal(t, 2, len(res.Query[0].Children[0].FacetsOrder))
+	require.Equal(t, "closeness", res.Query[0].Children[0].FacetsOrder[0].Key)
+	require.True(t, res.Query[0].Children[0].FacetsOrder[0].Desc)
+	require.Equal(t, "since", res.Query[0].Children[0].FacetsOrder[1].Key)
+	require.False(t, res.Query[0].Children[0].FacetsOrder[1].Desc)
+}
+
+func TestParseOrderbyMultipleFacetsWithAlias(t *testing.T) {
+	query := `
+	query {
+		me(func: uid(0x1)) {
+			friends @facets(orderdesc: closeness, orderasc: since, score, location:from) {
+				name
+			}
+		}
+	}
+`
+	res, err := Parse(Request{Str: query})
+	require.NoError(t, err)
+	require.NotNil(t, res.Query[0])
+	require.Equal(t, []string{"friends"}, childAttrs(res.Query[0]))
+	require.NotNil(t, res.Query[0].Children[0].Facets)
+	require.Equal(t, []string{"name"}, childAttrs(res.Query[0].Children[0]))
+	require.Equal(t, 2, len(res.Query[0].Children[0].FacetsOrder))
+	require.Equal(t, "closeness", res.Query[0].Children[0].FacetsOrder[0].Key)
+	require.True(t, res.Query[0].Children[0].FacetsOrder[0].Desc)
+	require.Equal(t, "since", res.Query[0].Children[0].FacetsOrder[1].Key)
+	require.False(t, res.Query[0].Children[0].FacetsOrder[1].Desc)
+	require.Equal(t, 4, len(res.Query[0].Children[0].Facets.Param))
+	require.Nil(t, res.Query[0].Children[0].FacetsFilter)
+	require.Empty(t, res.Query[0].Children[0].FacetVar)
+	for _, param := range res.Query[0].Children[0].Facets.Param {
+		if param.Key == "from" {
+			require.Equal(t, "location", param.Alias)
+			break
+		}
+	}
+}
+
+func TestParseOrderbySameFacetsMultipleTimes(t *testing.T) {
+	query := `
+	query {
+		me(func: uid(0x1)) {
+			friends @facets(orderdesc: closeness, orderasc: closeness) {
+				name
+			}
+		}
+	}
+`
+	_, err := Parse(Request{Str: query})
+	require.Contains(t, err.Error(),
+		"Sorting by facet: [closeness] can only be done once")
 }
 
 func TestParseOrderbyFacet(t *testing.T) {

query/query.go

@@ -30,7 +30,6 @@ import (
 	otrace "go.opencensus.io/trace"
 	"google.golang.org/grpc/metadata"
 
-	"github.com/dgraph-io/dgo/v2/protos/api"
 	"github.com/dgraph-io/dgraph/algo"
 	"github.com/dgraph-io/dgraph/gql"
 	"github.com/dgraph-io/dgraph/protos/pb"
@@ -126,11 +125,9 @@ type params struct {
 
 	// Facet tells us about the requested facets and their aliases.
 	Facet *pb.FacetParams
-	// FacetOrder has the name of the facet by which the results should be sorted.
-	FacetOrder string
-	// FacetOrderDesc is true if the facets should be order in descending order. If it's
-	// false, the facets will be ordered in ascending order.
-	FacetOrderDesc bool
+	// FacetsOrder keeps ordering for facets. Each entry stores name of the facet key and
+	// OrderDesc(will be true if results should be ordered by desc order of key) information for it.
+	FacetsOrder []*gql.FacetOrder
 
 	// Var is the name of the variable defined in this SubGraph
 	// (e.g. in "x as name", this would be x).
@@ -514,23 +511,22 @@ func treeCopy(gq *gql.GraphQuery, sg *SubGraph) error {
 		attrsSeen[key] = struct{}{}
 
 		args := params{
-			Alias:          gchild.Alias,
-			Cascade:        gchild.Cascade || sg.Params.Cascade,
-			Expand:         gchild.Expand,
-			Facet:          gchild.Facets,
-			FacetOrder:     gchild.FacetOrder,
-			FacetOrderDesc: gchild.FacetDesc,
-			FacetVar:       gchild.FacetVar,
-			GetUid:         sg.Params.GetUid,
-			IgnoreReflex:   sg.Params.IgnoreReflex,
-			Langs:          gchild.Langs,
-			NeedsVar:       append(gchild.NeedsVar[:0:0], gchild.NeedsVar...),
-			Normalize:      gchild.Normalize || sg.Params.Normalize,
-			Order:          gchild.Order,
-			Var:            gchild.Var,
-			GroupbyAttrs:   gchild.GroupbyAttrs,
-			IsGroupBy:      gchild.IsGroupby,
-			IsInternal:     gchild.IsInternal,
+			Alias:        gchild.Alias,
+			Cascade:      gchild.Cascade || sg.Params.Cascade,
+			Expand:       gchild.Expand,
+			Facet:        gchild.Facets,
+			FacetsOrder:  gchild.FacetsOrder,
+			FacetVar:     gchild.FacetVar,
+			GetUid:       sg.Params.GetUid,
+			IgnoreReflex: sg.Params.IgnoreReflex,
+			Langs:        gchild.Langs,
+			NeedsVar:     append(gchild.NeedsVar[:0:0], gchild.NeedsVar...),
+			Normalize:    gchild.Normalize || sg.Params.Normalize,
+			Order:        gchild.Order,
+			Var:          gchild.Var,
+			GroupbyAttrs: gchild.GroupbyAttrs,
+			IsGroupBy:    gchild.IsGroupby,
+			IsInternal:   gchild.IsInternal,
 		}
 
 		if gchild.IsCount {
@@ -549,7 +545,7 @@ func treeCopy(gq *gql.GraphQuery, sg *SubGraph) error {
 			return err
 		}
 
-		if len(args.Order) != 0 && len(args.FacetOrder) != 0 {
+		if len(args.Order) != 0 && len(args.FacetsOrder) != 0 {
 			return errors.Errorf("Cannot specify order at both args and facets")
 		}
 
@@ -1259,7 +1255,7 @@ func (sg *SubGraph) updateFacetMatrix() {
 func (sg *SubGraph) updateUidMatrix() {
 	sg.updateFacetMatrix()
 	for _, l := range sg.uidMatrix {
-		if len(sg.Params.Order) > 0 || len(sg.Params.FacetOrder) > 0 {
+		if len(sg.Params.Order) > 0 || len(sg.Params.FacetsOrder) > 0 {
 			// 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 {
@@ -2101,7 +2097,7 @@ func ProcessGraph(ctx context.Context, sg, parent *SubGraph, rch chan error) {
 		}
 	}
 
-	if len(sg.Params.Order) == 0 && len(sg.Params.FacetOrder) == 0 {
+	if len(sg.Params.Order) == 0 && len(sg.Params.FacetsOrder) == 0 {
 		// There is no ordering. Just apply pagination and return.
 		if err = sg.applyPagination(ctx); err != nil {
 			rch <- err
@@ -2235,14 +2231,14 @@ func (sg *SubGraph) applyPagination(ctx context.Context) error {
 // applyOrderAndPagination orders each posting list by a given attribute
 // before applying pagination.
 func (sg *SubGraph) applyOrderAndPagination(ctx context.Context) error {
-	if len(sg.Params.Order) == 0 && len(sg.Params.FacetOrder) == 0 {
+	if len(sg.Params.Order) == 0 && len(sg.Params.FacetsOrder) == 0 {
 		return nil
 	}
 
 	sg.updateUidMatrix()
 
 	// See if we need to apply order based on facet.
-	if len(sg.Params.FacetOrder) != 0 {
+	if len(sg.Params.FacetsOrder) != 0 {
 		return sg.sortAndPaginateUsingFacet(ctx)
 	}
 
@@ -2322,41 +2318,62 @@ func (sg *SubGraph) sortAndPaginateUsingFacet(ctx context.Context) error {
 		return errors.Errorf("Facet matrix and UID matrix mismatch: %d vs %d",
 			len(sg.facetsMatrix), len(sg.uidMatrix))
 	}
-	orderby := sg.Params.FacetOrder
+
+	orderbyKeys := make(map[string]int)
+	var orderDesc []bool
+	for i, order := range sg.Params.FacetsOrder {
+		orderbyKeys[order.Key] = i
+		orderDesc = append(orderDesc, order.Desc)
+	}
+
 	for i := 0; i < len(sg.uidMatrix); i++ {
 		ul := sg.uidMatrix[i]
 		fl := sg.facetsMatrix[i]
 		uids := ul.Uids[:0]
-		values := make([][]types.Val, 0, len(ul.Uids))
 		facetList := fl.FacetsList[:0]
+
+		values := make([][]types.Val, len(ul.Uids))
+		for i := 0; i < len(values); i++ {
+			values[i] = make([]types.Val, len(sg.Params.FacetsOrder))
+		}
+
 		for j := 0; j < len(ul.Uids); j++ {
-			var facet *api.Facet
 			uid := ul.Uids[j]
 			f := fl.FacetsList[j]
 			uids = append(uids, uid)
 			facetList = append(facetList, f)
+
+			// Since any facet can come only once in f.Facets, we can have counter to check if we
+			// have populated all facets or not. Once we are done populating all facets
+			// we can break out of below loop.
+			remainingFacets := len(orderbyKeys)
+			// TODO: We are searching sequentially, explore if binary search is useful here.
 			for _, it := range f.Facets {
-				if it.Key == orderby {
-					facet = it
-					break
+				idx, ok := orderbyKeys[it.Key]
+				if !ok {
+					continue
 				}
-			}
-			if facet != nil {
-				fVal, err := facets.ValFor(facet)
+
+				fVal, err := facets.ValFor(it)
 				if err != nil {
 					return err
 				}
+				if !types.IsSortable(fVal.Tid) {
+					return errors.Errorf("Value of type: %s isn't sortable", fVal.Tid.Name())
+				}
 
-				values = append(values, []types.Val{fVal})
-			} else {
-				values = append(values, []types.Val{{Value: nil}})
+				values[j][idx] = fVal
+				remainingFacets--
+				if remainingFacets == 0 {
+					break
+				}
 			}
 		}
 		if len(values) == 0 {
 			continue
 		}
-		if err := types.SortWithFacet(values, &uids, facetList,
-			[]bool{sg.Params.FacetOrderDesc}, ""); err != nil {
+
+		if err := types.SortWithFacet(values, &uids, facetList, orderDesc, ""); err != nil {
 			return err
 		}
 		sg.uidMatrix[i].Uids = uids