fix(sort): Fix multi-sort with nils (hypermodeinc#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.
brandonheng168 · Feb 17, 2021 · 2c7de57 · 2c7de57
1 parent 832ebb9
commit 2c7de57
Show file tree

Hide file tree

Showing 5 changed files with 214 additions and 8 deletions.
diff --git a/query/common_test.go b/query/common_test.go
@@ -786,6 +786,48 @@ func populateCluster() {
 		<69> <pname> "nameI" .
 		<70> <pname> "nameJ" .
 
+		<61> <pred1> "A" .
+		<62> <pred1> "A" .
+		<63> <pred1> "A" .
+		<64> <pred1> "B" .
+		<65> <pred1> "B" .
+		<66> <pred1> "B" .
+		<67> <pred1> "C" .
+		<68> <pred1> "C" .
+		<69> <pred1> "C" .
+		<70> <pred1> "C" .
+
+		<61> <pred2> "I" .
+		<62> <pred2> "J" .
+
+		<64> <pred2> "I" .
+		<65> <pred2> "J" .
+
+		<67> <pred2> "I" .
+		<68> <pred2> "J" .
+		<69> <pred2> "K" .
+
+
+		<61> <index-pred1> "A" .
+		<62> <index-pred1> "A" .
+		<63> <index-pred1> "A" .
+		<64> <index-pred1> "B" .
+		<65> <index-pred1> "B" .
+		<66> <index-pred1> "B" .
+		<67> <index-pred1> "C" .
+		<68> <index-pred1> "C" .
+		<69> <index-pred1> "C" .
+		<70> <index-pred1> "C" .
+
+		<61> <index-pred2> "I" .
+		<62> <index-pred2> "J" .
+
+		<64> <index-pred2> "I" .
+		<65> <index-pred2> "J" .
+
+		<67> <index-pred2> "I" .
+		<68> <index-pred2> "J" .
+		<69> <index-pred2> "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
@@ -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
@@ -599,16 +599,16 @@ func SortFacetsReturnNil(t *testing.T, c *dgo.Dgraph) {
 				{
 					"name":"Michael",
 					"friend":[
-						{
-							"name":"Charlie"
-						},
 						{
 							"name":"Alice",
 							"friend|since":"2014-01-02T00:00:00Z"
 						},
 						{
 							"name":"Sang Hyun",
 							"friend|since":"2012-01-02T00:00:00Z"
+						},
+						{
+							"name":"Charlie"
 						}
 					]
 				}

diff --git 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
@@ -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 {