fix: Make sort stable and handle nil comparison (#1094)

- Resolves #1071

- Resolves #921

- This change uses stable sort and ensures the `Less` interface implementation only returns true if the comparison is less, otherwise returns false (this is not the full story as for when we do `DESC` the less to us then is the check of if it's greater than instead of less than). Note: Ensures the ordering of the input array is preserved, so the output is always stable.

- This change works on both GoLang v1.18.5 and v1.19.5.

- This also resolves some nil panics we were having before (one subtask of #833, which is issue #921).
	1) Fixes the test: `TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc`
	2) Fixes the test: `TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

- Ensures our sort handles `nil` sorting properly for `ASC` and `DESC`:
```
DESC = [10, 9, 8, ... nil, nil]
ASC  = [nil, nil, 1, 2, 3, ... ]
```

db/base/compare.go

@@ -26,6 +26,10 @@ import (
 // and they are always the same type as each other.
 // @todo: Handle list/slice/array fields
 func Compare(a, b any) int {
+	if a == nil || b == nil {
+		return compareNil(a, b)
+	}
+
 	switch v := a.(type) {
 	case bool:
 		return compareBool(v, b.(bool))
@@ -48,6 +52,15 @@ func Compare(a, b any) int {
 	}
 }
 
+func compareNil(a, b any) int {
+	if a == nil && b == nil {
+		return 0
+	} else if b == nil && a != nil { // a > b (1 > nil)
+		return 1
+	}
+	return -1
+}
+
 func compareBool(a, b bool) int {
 	if a == b {
 		return 0

planner/values.go

@@ -77,39 +77,43 @@ func (n *valuesNode) Source() planNode { return nil }
 
 // SortAll actually sorts all the data within the docContainer object
 func (n *valuesNode) SortAll() {
-	sort.Sort(n)
+	sort.Stable(n)
 }
 
-// Less implements the golang sort.Sort interface.
-// It compares the values the ith and jth index
-// within the docContainer.
-// returns true if i < j.
-// returns false if i > j.
+// Less implements the golang sort.Interface.
+// Less reports whether the elements within the docContainer at index i must sort before the element with index j.
+// Returns true if docs[i] < docs[j].
+// Returns false if docs[i] >= docs[j].
+// If both Less(i, j) and Less(j, i) are false, then the elements at index i and j are considered equal.
 func (n *valuesNode) Less(i, j int) bool {
 	da, db := n.docs.At(i), n.docs.At(j)
 	return n.docValueLess(da, db)
 }
 
-// docValueLess extracts and compare field values of a document
-func (n *valuesNode) docValueLess(da, db core.Doc) bool {
-	var ra, rb any
+// docValueLess extracts and compare field values of a document, returns true only if strictly less when ASC,
+// and true if greater than or equal when DESC, otherwise returns false.
+func (n *valuesNode) docValueLess(docA, docB core.Doc) bool {
 	for _, order := range n.ordering {
-		if order.Direction == mapper.ASC {
-			ra = getDocProp(da, order.FieldIndexes)
-			rb = getDocProp(db, order.FieldIndexes)
-		} else if order.Direction == mapper.DESC { // redundant, just else
-			ra = getDocProp(db, order.FieldIndexes)
-			rb = getDocProp(da, order.FieldIndexes)
-		}
-
-		if c := base.Compare(ra, rb); c < 0 {
-			return true
-		} else if c > 0 {
-			return false
+		compare := base.Compare(
+			getDocProp(docA, order.FieldIndexes),
+			getDocProp(docB, order.FieldIndexes),
+		)
+
+		if order.Direction == mapper.DESC {
+			if compare > 0 {
+				return true
+			} else {
+				return false
+			}
+		} else { // Otherwise assume order.Direction == mapper.ASC
+			if compare < 0 {
+				return true
+			} else {
+				return false
+			}
 		}
 	}
-
-	return true
+	return false
 }
 
 // Swap implements the golang sort.Sort interface.

tests/integration/query/commits/with_dockey_order_test.go

@@ -44,23 +44,23 @@ func TestQueryCommitsWithDockeyAndOrderHeightDesc(t *testing.T) {
 		},
 		Results: []map[string]any{
 			{
-				"cid":    "bafybeibz3vbkt75siz3zogke6tlzvpcxttpiy4xivjvgyeaorjz6wsbguq",
+				"cid":    "bafybeiacqac6scm7pmtlvqptvtljmoroevnoedku42qi5bmfdpaelcu5fm",
 				"height": int64(2),
 			},
 			{
-				"cid":    "bafybeiacqac6scm7pmtlvqptvtljmoroevnoedku42qi5bmfdpaelcu5fm",
+				"cid":    "bafybeibz3vbkt75siz3zogke6tlzvpcxttpiy4xivjvgyeaorjz6wsbguq",
 				"height": int64(2),
 			},
 			{
-				"cid":    "bafybeid5l577igkgcn6wjqjeqxlta4dcc3a3iykwkborf4fklaenjuctoq",
+				"cid":    "bafybeigju7dgicfq3fxvtlxtjao7won4xc7kusykkvumngjfx5i2c7ibny",
 				"height": int64(1),
 			},
 			{
 				"cid":    "bafybeiaqarrcayyoly2gdiam6mhh72ls4azwa7brozxxc3q2srnggkkqkq",
 				"height": int64(1),
 			},
 			{
-				"cid":    "bafybeigju7dgicfq3fxvtlxtjao7won4xc7kusykkvumngjfx5i2c7ibny",
+				"cid":    "bafybeid5l577igkgcn6wjqjeqxlta4dcc3a3iykwkborf4fklaenjuctoq",
 				"height": int64(1),
 			},
 		},
@@ -97,23 +97,23 @@ func TestQueryCommitsWithDockeyAndOrderHeightAsc(t *testing.T) {
 		},
 		Results: []map[string]any{
 			{
-				"cid":    "bafybeid5l577igkgcn6wjqjeqxlta4dcc3a3iykwkborf4fklaenjuctoq",
+				"cid":    "bafybeigju7dgicfq3fxvtlxtjao7won4xc7kusykkvumngjfx5i2c7ibny",
 				"height": int64(1),
 			},
 			{
 				"cid":    "bafybeiaqarrcayyoly2gdiam6mhh72ls4azwa7brozxxc3q2srnggkkqkq",
 				"height": int64(1),
 			},
 			{
-				"cid":    "bafybeigju7dgicfq3fxvtlxtjao7won4xc7kusykkvumngjfx5i2c7ibny",
+				"cid":    "bafybeid5l577igkgcn6wjqjeqxlta4dcc3a3iykwkborf4fklaenjuctoq",
 				"height": int64(1),
 			},
 			{
-				"cid":    "bafybeibz3vbkt75siz3zogke6tlzvpcxttpiy4xivjvgyeaorjz6wsbguq",
+				"cid":    "bafybeiacqac6scm7pmtlvqptvtljmoroevnoedku42qi5bmfdpaelcu5fm",
 				"height": int64(2),
 			},
 			{
-				"cid":    "bafybeiacqac6scm7pmtlvqptvtljmoroevnoedku42qi5bmfdpaelcu5fm",
+				"cid":    "bafybeibz3vbkt75siz3zogke6tlzvpcxttpiy4xivjvgyeaorjz6wsbguq",
 				"height": int64(2),
 			},
 		},
@@ -227,3 +227,78 @@ func TestQueryCommitsWithDockeyAndOrderCidAsc(t *testing.T) {
 
 	executeTestCase(t, test)
 }
+
+func TestQueryCommitsWithDockeyAndOrderAndMultiUpdatesCidAsc(t *testing.T) {
+	test := testUtils.RequestTestCase{
+		Description: "Simple all commits query with dockey, multiple updates with order cid asc",
+		Request: `query {
+                     commits(dockey: "bae-52b9170d-b77a-5887-b877-cbdbb99b009f", order: {height: ASC}) {
+                         cid
+                         height
+                     }
+                 }`,
+		Docs: map[int][]string{
+			0: {
+				`{
+                     "Name": "John",
+                     "Age": 21
+                 }`,
+			},
+		},
+		Updates: map[int]map[int][]string{
+			0: {
+				0: {
+					`{
+                         "Age": 22
+                     }`,
+					`{
+                         "Age": 23
+                     }`,
+					`{
+                         "Age": 24
+                     }`,
+				},
+			},
+		},
+		Results: []map[string]any{
+			{
+				"cid":    "bafybeigju7dgicfq3fxvtlxtjao7won4xc7kusykkvumngjfx5i2c7ibny",
+				"height": int64(1),
+			},
+			{
+				"cid":    "bafybeiaqarrcayyoly2gdiam6mhh72ls4azwa7brozxxc3q2srnggkkqkq",
+				"height": int64(1),
+			},
+			{
+				"cid":    "bafybeid5l577igkgcn6wjqjeqxlta4dcc3a3iykwkborf4fklaenjuctoq",
+				"height": int64(1),
+			},
+			{
+				"cid":    "bafybeiacqac6scm7pmtlvqptvtljmoroevnoedku42qi5bmfdpaelcu5fm",
+				"height": int64(2),
+			},
+			{
+				"cid":    "bafybeibz3vbkt75siz3zogke6tlzvpcxttpiy4xivjvgyeaorjz6wsbguq",
+				"height": int64(2),
+			},
+			{
+				"cid":    "bafybeiho5z6seahwxgbdyobylzyarrdschgzmood7rkdtp4qpd2uxebaxy",
+				"height": int64(3),
+			},
+			{
+				"cid":    "bafybeib6yxcmbg2gz5ss6d67u5mu6wcatfjtdp2rv44difyznp3rqlyu4m",
+				"height": int64(3),
+			},
+			{
+				"cid":    "bafybeihhlr6teynoy534b32kihulsmtdnnzp5woebvhubvofoxqx4g4lha",
+				"height": int64(4),
+			},
+			{
+				"cid":    "bafybeicgc6k47su6263dm3mv5j5gkv3bbt43nb66x6wjashjxpfm2ds5nu",
+				"height": int64(4),
+			},
+		},
+	}
+
+	executeTestCase(t, test)
+}

tests/integration/query/one_to_many_to_one/with_order_limit_test.go

@@ -16,10 +16,10 @@ import (
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
-// TODO: Fix this panic in #833.
 func TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc(t *testing.T) {
 	test := testUtils.RequestTestCase{
 		Description: "1-N-1 deep orderby subtypes of both descending and ascending.",
+
 		Request: `query {
 		    Author {
 				name
@@ -129,8 +129,41 @@ func TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc(t *testing.T) {
 			    }`,
 			},
 		},
-		Results: []map[string]any{},
+
+		Results: []map[string]any{
+			{
+				"name": "John Grisham",
+				"NewestPublishersBook": []map[string]any{
+					{
+						"name": "Theif Lord",
+					},
+				},
+				"OldestPublishersBook": []map[string]any{
+					{
+						"name": "The Associate", // oldest because has no publisher.
+					},
+				},
+			},
+			{
+				"name":                 "Not a Writer",
+				"NewestPublishersBook": []map[string]any{},
+				"OldestPublishersBook": []map[string]any{},
+			},
+			{
+				"name": "Cornelia Funke",
+				"NewestPublishersBook": []map[string]any{
+					{
+						"name": "The Rooster Bar",
+					},
+				},
+				"OldestPublishersBook": []map[string]any{
+					{
+						"name": "The Rooster Bar",
+					},
+				},
+			},
+		},
 	}
 
-	testUtils.AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })
+	executeTestCase(t, test)
 }

tests/integration/query/one_to_many_to_one/with_order_test.go

@@ -299,19 +299,6 @@ func TestMultipleOrderByWithDepthGreaterThanOneOrderSwitched(t *testing.T) {
 					"yearOpened": uint64(2022),
 				},
 			},
-			{
-				"name":   "Sooley",
-				"rating": 3.2,
-				"publisher": map[string]any{
-					"name":       "Only Publisher of Sooley",
-					"yearOpened": uint64(1999),
-				},
-			},
-			{
-				"name":      "The Associate",
-				"rating":    4.2,
-				"publisher": nil,
-			},
 			{
 				"name":   "Theif Lord",
 				"rating": 4.8,
@@ -328,6 +315,14 @@ func TestMultipleOrderByWithDepthGreaterThanOneOrderSwitched(t *testing.T) {
 					"yearOpened": uint64(2013),
 				},
 			},
+			{
+				"name":   "Sooley",
+				"rating": 3.2,
+				"publisher": map[string]any{
+					"name":       "Only Publisher of Sooley",
+					"yearOpened": uint64(1999),
+				},
+			},
 			{
 				"name":   "Painted House",
 				"rating": 4.9,
@@ -336,6 +331,11 @@ func TestMultipleOrderByWithDepthGreaterThanOneOrderSwitched(t *testing.T) {
 					"yearOpened": uint64(1995),
 				},
 			},
+			{
+				"name":      "The Associate",
+				"rating":    4.2,
+				"publisher": nil,
+			},
 		},
 	}
 

tests/integration/query/one_to_many_to_one/with_sum_order_limit_test.go

@@ -159,10 +159,10 @@ func TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeDescDirec
 	executeTestCase(t, test)
 }
 
-// TODO: Fix this panic in #833.
 func TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirections(t *testing.T) {
 	test := testUtils.RequestTestCase{
 		Description: "1-N-1 sum of deep orderby subtypes and non-sum deep orderby, asc. directions.",
+
 		Request: `query {
  		    Author {
  				name
@@ -270,16 +270,21 @@ func TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirect
  			    }`,
 			},
 		},
+
 		Results: []map[string]any{
 			{
 				"name": "John Grisham",
-				"s1":   4.9 + 3.2, // Because in ascending order years for John are [1995, 1999].
+
+				// Because in ascending order years for John are:
+				// 'The Associate' as it has no publisher (4.2 rating), then 'Painted House' 1995 (4.9 rating).
+				"s1": float64(4.2) + float64(4.9),
+
 				"NewestPublishersBook": []map[string]any{
 					{
-						"name": "Painted House",
+						"name": "The Associate",
 					},
 					{
-						"name": "Sooley",
+						"name": "Painted House",
 					},
 				},
 			},
@@ -300,13 +305,14 @@ func TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirect
 		},
 	}
 
-	testUtils.AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })
+	executeTestCase(t, test)
 }
 
-// TODO: Fix this panic in #833.
+// TODO: Fix this panic in #833 and #920.
 func TestOneToManyToOneWithSumOfDeepOrderBySubTypeOfBothDescAndAsc(t *testing.T) {
 	test := testUtils.RequestTestCase{
 		Description: "1-N-1 sums of deep orderby subtypes of both descending and ascending.",
+
 		Request: `query {
 		    Author {
 				name
@@ -418,7 +424,7 @@ func TestOneToManyToOneWithSumOfDeepOrderBySubTypeOfBothDescAndAsc(t *testing.T)
 	testUtils.AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })
 }
 
-// TODO: Fix this panic in #833.
+// TODO: Fix this panic in #833 and #920.
 func TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeOppositeDirections(t *testing.T) {
 	test := testUtils.RequestTestCase{
 		Description: "1-N-1 sum of deep orderby subtypes and non-sum deep orderby, opposite directions.",