Multiple/sort

[pawanrawal]

This change is 

[manishrjain]

---

Reviewed 3 of 9 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

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

	Args map[string]string
	// Query can have multiple sort parameters.
	OrderAttr    []string

Create a struct around these two fields.

---

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

	for _, it := range sg.Params.NeedsVar {
		// TODO(pawan) - Return error if user uses var order with predicates.
		if len(sg.Params.OrderAttr) > 0 && it.Name == sg.Params.OrderAttr[0] && (it.Typ == gql.VALUE_VAR) {

100 chars.

---

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

// Less compares two elements
func (s byValue) Less(i, j int) bool {
	if len(s.values[i]) == 0 || len(s.values[j]) == 0 {

assign the i'th and j'th element to vars upfront.

---

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

	}

	for idx, _ := range s.values[i] {

vidx

---

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

		// and at last place while doing ascending sort.
		if s.values[i][idx].Value == nil {
			if s.desc[idx] {

return s.desc[idx]

---

worker/sort.go, line 321 at r2 (raw file):

	// SrcUids for other queries are all the uids present in the response of the first sort.
	destUids := destUids(r.reply.UidMatrix)

Refactor this logic out into a separate function.

---

worker/sort.go, line 445 at r2 (raw file):

	}
	r, err := ProcessTaskOverNetwork(ctx, in)
	select {

No need for select here.

---

Comments from Reviewable

[janardhan1993]

Reviewed 3 of 9 files at r1, 6 of 7 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.

---

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

	expectArg := true
	order := make(map[string]bool)

map[string]struct{}

---

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

	for _, it := range sg.Params.NeedsVar {
		// TODO(pawan) - Return error if user uses var order with predicates.
		if len(sg.Params.OrderAttr) > 0 && it.Name == sg.Params.OrderAttr[0] && (it.Typ == gql.VALUE_VAR) {

Check this at the top level itself, either all var or all predicates.

---

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

func (sg *SubGraph) sortAndPaginateUsingVar(ctx context.Context) error {
	if sg.Params.uidToVal == nil {
		return x.Errorf("Variable: [%s] used before definition.", sg.Params.OrderAttr[0])

We don't do multiple var ??

---

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

		return false
	}
	if s.values[i][0].Tid != s.values[j][0].Tid {

Everything should be converted to schema type before calling this. So ideally they shouldn't be of different type for same predicate.

---

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

		// Null value is considered greatest hence comes at first place while doing descending sort
		// and at last place while doing ascending sort.
		if s.values[i][idx].Value == nil {

Do we have same behaviour in sortandpaginateusing vars ?, keep all the uids with null value at the end.

---

Comments from Reviewable

[janardhan1993]

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

---

worker/sort.go, line 45 at r2 (raw file):

type sortresult struct {
	reply *protos.SortResult
	vals  [][]types.Val

We don't need this if it's single attribute ?

---

worker/sort.go, line 364 at r2 (raw file):

	for i := 1; i < len(ts.Attr); i++ {
		or := <-och
		if or.err != nil && oerr == nil {

shouldn't it be break ?

---

worker/sort.go, line 371 at r2 (raw file):

		result := or.r
		x.AssertTrue(len(result.ValueMatrix) == len(destUids.Uids))
		seen = map[uint64]bool{}

aren't destUids deduplicated ? why do we need seen.

---

Comments from Reviewable

[pawanrawal]

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

---

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

Previously, manishrjain (Manish R Jain) wrote…

Create a struct around these two fields.

Done.

---

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

Previously, janardhan1993 (Janardhan Reddy) wrote…

map[string]struct{}

Is that better? I think thats more useful if I have some starting values.

---

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

Previously, janardhan1993 (Janardhan Reddy) wrote…

Check this at the top level itself, either all var or all predicates.

Will do this later. Have a TODO for it.

---

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

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.

---

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

Previously, janardhan1993 (Janardhan Reddy) wrote…

We don't do multiple var ??

No

---

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

Previously, manishrjain (Manish R Jain) wrote…

assign the i'th and j'th element to vars upfront.

Done.

---

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

Previously, janardhan1993 (Janardhan Reddy) wrote…

Everything should be converted to schema type before calling this. So ideally they shouldn't be of different type for same predicate.

Yeah we dont need this check

---

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

Previously, manishrjain (Manish R Jain) wrote…

vidx

Done.

---

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

Previously, janardhan1993 (Janardhan Reddy) wrote…

Do we have same behaviour in sortandpaginateusing vars ?, keep all the uids with null value at the end.

I think we just dont consider them.

---

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

Previously, manishrjain (Manish R Jain) wrote…

return s.desc[idx]

Done.

---

worker/sort.go, line 45 at r2 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

We don't need this if it's single attribute ?

Yes but I need to have a common API.

---

worker/sort.go, line 321 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Refactor this logic out into a separate function.

Done.

---

worker/sort.go, line 364 at r2 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

shouldn't it be break ?

Waiting for all of them to finish, else trace issue.

---

worker/sort.go, line 371 at r2 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

aren't destUids deduplicated ? why do we need seen.

Actually we dont need it here.

---

worker/sort.go, line 445 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need for select here.

Done

---

Comments from Reviewable

[janardhan1993]

Review status: 1 of 11 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

I think we just dont consider them.

Shouldn't we have same behaviour there also with multiple vars.

---

worker/sort.go, line 364 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Waiting for all of them to finish, else trace issue.

We continue on first error but on next error we do processing which we don't need to
if oerr != nil {
continue
} ??

---

Comments from Reviewable

[janardhan1993]

---

Review status: 1 of 11 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[pawanrawal]

This was merged to master.