Skip to content

Commit

Permalink
Add support for sorting on multiple facets (#4579)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ashish-goswami
ashish-goswami authored Jan 20, 2020
1 parent 436c78e commit c23827e
Show file tree
Hide file tree
Showing 6 changed files with 444 additions and 68 deletions.
38 changes: 21 additions & 17 deletions gql/parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.

Expand All @@ -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})
}
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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")
}
Expand Down
74 changes: 72 additions & 2 deletions gql/parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
101 changes: 59 additions & 42 deletions query/query.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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 {
Expand All @@ -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")
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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 type is not sortable, we are ignoring it.
if types.IsSortable(fVal.Tid) {
values[j][idx] = fVal
}

values = append(values, []types.Val{fVal})
} else {
values = append(values, []types.Val{{Value: nil}})
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
Expand Down
Loading

0 comments on commit c23827e

Please sign in to comment.