From 2c7de573090f700841b2d9bda2779c48349fedc5 Mon Sep 17 00:00:00 2001 From: Ahsan Barkati Date: Wed, 17 Feb 2021 19:25:34 +0530 Subject: [PATCH] fix(sort): Fix multi-sort with nils (#7432) Fix the behavior of multi-sort. All the nil values are appended at the end of the result irrespective of ascending or descending sort. This change also fixes a bug due to not appending the nil values in the values list, corresponding to the UIDs appended in the UID list with nil predicates. --- query/common_test.go | 42 ++++++++++ query/query1_test.go | 156 +++++++++++++++++++++++++++++++++++++- systest/mutations_test.go | 6 +- types/sort.go | 12 ++- worker/sort.go | 6 ++ 5 files changed, 214 insertions(+), 8 deletions(-) diff --git a/query/common_test.go b/query/common_test.go index f5e0c6c9af1..34b93e65823 100644 --- a/query/common_test.go +++ b/query/common_test.go @@ -786,6 +786,48 @@ func populateCluster() { <69> "nameI" . <70> "nameJ" . + <61> "A" . + <62> "A" . + <63> "A" . + <64> "B" . + <65> "B" . + <66> "B" . + <67> "C" . + <68> "C" . + <69> "C" . + <70> "C" . + + <61> "I" . + <62> "J" . + + <64> "I" . + <65> "J" . + + <67> "I" . + <68> "J" . + <69> "K" . + + + <61> "A" . + <62> "A" . + <63> "A" . + <64> "B" . + <65> "B" . + <66> "B" . + <67> "C" . + <68> "C" . + <69> "C" . + <70> "C" . + + <61> "I" . + <62> "J" . + + <64> "I" . + <65> "J" . + + <67> "I" . + <68> "J" . + <69> "K" . `) if err != nil { panic(fmt.Sprintf("Could not able add triple to the cluster. Got error %v", err.Error())) diff --git a/query/query1_test.go b/query/query1_test.go index 5377fbab009..7ad83ae8e77 100644 --- a/query/query1_test.go +++ b/query/query1_test.go @@ -1947,7 +1947,7 @@ func TestMultiSort5(t *testing.T) { }` js := processQueryNoErr(t, query) // Null value for third Alice comes at first. - require.JSONEq(t, `{"data": {"me":[{"name":"Alice","age":75},{"name":"Alice","age":75,"salary":10002.000000},{"name":"Alice","age":25,"salary":10000.000000},{"name":"Bob","age":25},{"name":"Bob","age":75},{"name":"Colin","age":25},{"name":"Elizabeth","age":25},{"name":"Elizabeth","age":75}]}}`, js) + require.JSONEq(t, `{"data": {"me":[{"name":"Alice","age":75,"salary":10002.000000},{"name":"Alice","age":25,"salary":10000.000000},{"name":"Alice","age":75},{"name":"Bob","age":25},{"name":"Bob","age":75},{"name":"Colin","age":25},{"name":"Elizabeth","age":25},{"name":"Elizabeth","age":75}]}}`, js) } func TestMultiSort6Paginate(t *testing.T) { @@ -2128,6 +2128,160 @@ func TestSortWithNulls(t *testing.T) { } } +func TestMultiSortWithNulls(t *testing.T) { + + tests := []struct { + index int32 + offset int32 + first int32 + desc bool + result string + }{ + {0, -1, -1, true, `{"data": {"me":[ + {"pname":"nameB","pred1":"A", "pred2":"J"}, + {"pname":"nameA","pred1":"A", "pred2":"I"}, + {"pname":"nameC","pred1":"A"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {1, -1, -1, false, `{"data": {"me":[ + {"pname":"nameA","pred1":"A", "pred2":"I"}, + {"pname":"nameB","pred1":"A", "pred2":"J"}, + {"pname":"nameC","pred1":"A"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {2, -1, 2, true, `{"data": {"me":[ + {"pname":"nameB","pred1":"A", "pred2":"J"}, + {"pname":"nameA","pred1":"A", "pred2":"I"}]}}`, + }, + {3, -1, 2, false, `{"data": {"me":[ + {"pname":"nameA","pred1":"A", "pred2":"I"}, + {"pname":"nameB","pred1":"A", "pred2":"J"}]}}`, + }, + {4, -1, 7, true, `{"data": {"me":[ + {"pname":"nameB","pred1":"A", "pred2":"J"}, + {"pname":"nameA","pred1":"A", "pred2":"I"}, + {"pname":"nameC","pred1":"A"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}]}}`, + }, + {5, -1, 7, false, `{"data": {"me":[ + {"pname":"nameA","pred1":"A", "pred2":"I"}, + {"pname":"nameB","pred1":"A", "pred2":"J"}, + {"pname":"nameC","pred1":"A"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}]}}`, + }, + {6, 2, 7, true, `{"data": {"me":[ + {"pname":"nameC","pred1":"A"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}]}}`, + }, + {7, 2, 7, false, `{"data": {"me":[ + {"pname":"nameC","pred1":"A"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}]}}`, + }, + {8, 2, 100, true, `{"data": {"me":[ + {"pname":"nameC","pred1":"A"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {9, 2, 100, false, `{"data": {"me":[ + {"pname":"nameC","pred1":"A"}, + {"pname":"nameD","pred1":"B", "pred2":"I"}, + {"pname":"nameE","pred1":"B", "pred2":"J"}, + {"pname":"nameF","pred1":"B"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {10, 5, 5, true, `{"data": {"me":[ + {"pname":"nameF","pred1":"B"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {11, 5, 5, false, `{"data": {"me":[ + {"pname":"nameF","pred1":"B"}, + {"pname":"nameG","pred1":"C", "pred2":"I"}, + {"pname":"nameH","pred1":"C", "pred2":"J"}, + {"pname":"nameI","pred1":"C", "pred2":"K"}, + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {12, 9, 5, true, `{"data": {"me":[ + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {13, 9, 5, false, `{"data": {"me":[ + {"pname":"nameJ","pred1":"C"}]}}`, + }, + {14, 12, 5, true, `{"data": {"me":[]}}`}, + {15, 12, 5, false, `{"data": {"me":[]}}`}, + } + makeQuery := func(offset, first int32, desc, index bool) string { + pred1 := "pred1" + pred2 := "pred2" + if index { + pred1 = "index-pred1" + pred2 = "index-pred2" + } + order := ",orderasc: " + if desc { + order = ",orderdesc: " + } + q := "me(func: uid(61, 62, 63, 64, 65, 66, 67, 68, 69, 70), orderasc: " + pred1 + + order + pred2 + if offset != -1 { + q += fmt.Sprintf(", offset: %d", offset) + } + if first != -1 { + q += fmt.Sprintf(", first: %d", first) + } + query := "{" + q + ") { pname pred1:" + pred1 + " pred2:" + pred2 + " } }" + return processQueryNoErr(t, query) + } + + for _, tc := range tests { + // Case of sort with Index. + actual := makeQuery(tc.offset, tc.first, tc.desc, true) + require.JSONEqf(t, tc.result, actual, "Failed on index-testcase: %d\n", tc.index) + + // Case of sort without index + actual = makeQuery(tc.offset, tc.first, tc.desc, false) + require.JSONEqf(t, tc.result, actual, "Failed on testcase: %d\n", tc.index) + } +} + func TestMultiSortPaginateWithOffset(t *testing.T) { t.Parallel() tests := []struct { diff --git a/systest/mutations_test.go b/systest/mutations_test.go index 29b87c60205..a4a763d4c7a 100644 --- a/systest/mutations_test.go +++ b/systest/mutations_test.go @@ -599,9 +599,6 @@ func SortFacetsReturnNil(t *testing.T, c *dgo.Dgraph) { { "name":"Michael", "friend":[ - { - "name":"Charlie" - }, { "name":"Alice", "friend|since":"2014-01-02T00:00:00Z" @@ -609,6 +606,9 @@ func SortFacetsReturnNil(t *testing.T, c *dgo.Dgraph) { { "name":"Sang Hyun", "friend|since":"2012-01-02T00:00:00Z" + }, + { + "name":"Charlie" } ] } diff --git a/types/sort.go b/types/sort.go index de5012086ac..13a320d1c6e 100644 --- a/types/sort.go +++ b/types/sort.go @@ -59,14 +59,18 @@ func (s byValue) Less(i, j int) bool { return false } for vidx := range first { - // Null value is considered greatest hence comes at first place while doing descending sort - // and at last place while doing ascending sort. - if first[vidx].Value == nil { + // Null values are appended at the end of the sort result for both ascending and descending. + // If both first and second has nil values, then maintain the order by UID. + if first[vidx].Value == nil && second[vidx].Value == nil { return s.desc[vidx] } + if first[vidx].Value == nil { + return false + } + if second[vidx].Value == nil { - return !s.desc[vidx] + return true } // We have to look at next value to decide. diff --git a/worker/sort.go b/worker/sort.go index de220d6176b..2ccb49d48e8 100644 --- a/worker/sort.go +++ b/worker/sort.go @@ -336,6 +336,12 @@ BUCKETS: remainingCount := int(ts.Count) - len(r.UidMatrix[i].Uids) canAppend := x.Min(uint64(remainingCount), uint64(len(nullNodes))) r.UidMatrix[i].Uids = append(r.UidMatrix[i].Uids, nullNodes[:canAppend]...) + + // The value list also need to contain null values for the appended uids. + if len(ts.Order) > 1 { + nullVals := make([]types.Val, canAppend) + values[i] = append(values[i], nullVals...) + } } select {