Skip to content

Commit b3412b2

Browse files
ahsanbarkatiahsanbarkati
committed
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. (cherry picked from commit 2c7de57)
1 parent 071b872 commit b3412b2

File tree

5 files changed

+214
-8
lines changed

5 files changed

+214
-8
lines changed

‎query/common_test.go

+42
Original file line numberDiff line numberDiff line change
@@ -786,6 +786,48 @@ func populateCluster() {
786786
<69> <pname> "nameI" .
787787
<70> <pname> "nameJ" .
788788
789+
<61> <pred1> "A" .
790+
<62> <pred1> "A" .
791+
<63> <pred1> "A" .
792+
<64> <pred1> "B" .
793+
<65> <pred1> "B" .
794+
<66> <pred1> "B" .
795+
<67> <pred1> "C" .
796+
<68> <pred1> "C" .
797+
<69> <pred1> "C" .
798+
<70> <pred1> "C" .
799+
800+
<61> <pred2> "I" .
801+
<62> <pred2> "J" .
802+
803+
<64> <pred2> "I" .
804+
<65> <pred2> "J" .
805+
806+
<67> <pred2> "I" .
807+
<68> <pred2> "J" .
808+
<69> <pred2> "K" .
809+
810+
811+
<61> <index-pred1> "A" .
812+
<62> <index-pred1> "A" .
813+
<63> <index-pred1> "A" .
814+
<64> <index-pred1> "B" .
815+
<65> <index-pred1> "B" .
816+
<66> <index-pred1> "B" .
817+
<67> <index-pred1> "C" .
818+
<68> <index-pred1> "C" .
819+
<69> <index-pred1> "C" .
820+
<70> <index-pred1> "C" .
821+
822+
<61> <index-pred2> "I" .
823+
<62> <index-pred2> "J" .
824+
825+
<64> <index-pred2> "I" .
826+
<65> <index-pred2> "J" .
827+
828+
<67> <index-pred2> "I" .
829+
<68> <index-pred2> "J" .
830+
<69> <index-pred2> "K" .
789831
`)
790832
if err != nil {
791833
panic(fmt.Sprintf("Could not able add triple to the cluster. Got error %v", err.Error()))

‎query/query1_test.go

+155-1
Original file line numberDiff line numberDiff line change
@@ -1887,7 +1887,7 @@ func TestMultiSort5(t *testing.T) {
18871887
}`
18881888
js := processQueryNoErr(t, query)
18891889
// Null value for third Alice comes at first.
1890-
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)
1890+
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)
18911891
}
18921892

18931893
func TestMultiSort6Paginate(t *testing.T) {
@@ -2068,6 +2068,160 @@ func TestSortWithNulls(t *testing.T) {
20682068
}
20692069
}
20702070

2071+
func TestMultiSortWithNulls(t *testing.T) {
2072+
2073+
tests := []struct {
2074+
index int32
2075+
offset int32
2076+
first int32
2077+
desc bool
2078+
result string
2079+
}{
2080+
{0, -1, -1, true, `{"data": {"me":[
2081+
{"pname":"nameB","pred1":"A", "pred2":"J"},
2082+
{"pname":"nameA","pred1":"A", "pred2":"I"},
2083+
{"pname":"nameC","pred1":"A"},
2084+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2085+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2086+
{"pname":"nameF","pred1":"B"},
2087+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2088+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2089+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2090+
{"pname":"nameJ","pred1":"C"}]}}`,
2091+
},
2092+
{1, -1, -1, false, `{"data": {"me":[
2093+
{"pname":"nameA","pred1":"A", "pred2":"I"},
2094+
{"pname":"nameB","pred1":"A", "pred2":"J"},
2095+
{"pname":"nameC","pred1":"A"},
2096+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2097+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2098+
{"pname":"nameF","pred1":"B"},
2099+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2100+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2101+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2102+
{"pname":"nameJ","pred1":"C"}]}}`,
2103+
},
2104+
{2, -1, 2, true, `{"data": {"me":[
2105+
{"pname":"nameB","pred1":"A", "pred2":"J"},
2106+
{"pname":"nameA","pred1":"A", "pred2":"I"}]}}`,
2107+
},
2108+
{3, -1, 2, false, `{"data": {"me":[
2109+
{"pname":"nameA","pred1":"A", "pred2":"I"},
2110+
{"pname":"nameB","pred1":"A", "pred2":"J"}]}}`,
2111+
},
2112+
{4, -1, 7, true, `{"data": {"me":[
2113+
{"pname":"nameB","pred1":"A", "pred2":"J"},
2114+
{"pname":"nameA","pred1":"A", "pred2":"I"},
2115+
{"pname":"nameC","pred1":"A"},
2116+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2117+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2118+
{"pname":"nameF","pred1":"B"},
2119+
{"pname":"nameI","pred1":"C", "pred2":"K"}]}}`,
2120+
},
2121+
{5, -1, 7, false, `{"data": {"me":[
2122+
{"pname":"nameA","pred1":"A", "pred2":"I"},
2123+
{"pname":"nameB","pred1":"A", "pred2":"J"},
2124+
{"pname":"nameC","pred1":"A"},
2125+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2126+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2127+
{"pname":"nameF","pred1":"B"},
2128+
{"pname":"nameG","pred1":"C", "pred2":"I"}]}}`,
2129+
},
2130+
{6, 2, 7, true, `{"data": {"me":[
2131+
{"pname":"nameC","pred1":"A"},
2132+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2133+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2134+
{"pname":"nameF","pred1":"B"},
2135+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2136+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2137+
{"pname":"nameG","pred1":"C", "pred2":"I"}]}}`,
2138+
},
2139+
{7, 2, 7, false, `{"data": {"me":[
2140+
{"pname":"nameC","pred1":"A"},
2141+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2142+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2143+
{"pname":"nameF","pred1":"B"},
2144+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2145+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2146+
{"pname":"nameI","pred1":"C", "pred2":"K"}]}}`,
2147+
},
2148+
{8, 2, 100, true, `{"data": {"me":[
2149+
{"pname":"nameC","pred1":"A"},
2150+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2151+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2152+
{"pname":"nameF","pred1":"B"},
2153+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2154+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2155+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2156+
{"pname":"nameJ","pred1":"C"}]}}`,
2157+
},
2158+
{9, 2, 100, false, `{"data": {"me":[
2159+
{"pname":"nameC","pred1":"A"},
2160+
{"pname":"nameD","pred1":"B", "pred2":"I"},
2161+
{"pname":"nameE","pred1":"B", "pred2":"J"},
2162+
{"pname":"nameF","pred1":"B"},
2163+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2164+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2165+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2166+
{"pname":"nameJ","pred1":"C"}]}}`,
2167+
},
2168+
{10, 5, 5, true, `{"data": {"me":[
2169+
{"pname":"nameF","pred1":"B"},
2170+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2171+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2172+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2173+
{"pname":"nameJ","pred1":"C"}]}}`,
2174+
},
2175+
{11, 5, 5, false, `{"data": {"me":[
2176+
{"pname":"nameF","pred1":"B"},
2177+
{"pname":"nameG","pred1":"C", "pred2":"I"},
2178+
{"pname":"nameH","pred1":"C", "pred2":"J"},
2179+
{"pname":"nameI","pred1":"C", "pred2":"K"},
2180+
{"pname":"nameJ","pred1":"C"}]}}`,
2181+
},
2182+
{12, 9, 5, true, `{"data": {"me":[
2183+
{"pname":"nameJ","pred1":"C"}]}}`,
2184+
},
2185+
{13, 9, 5, false, `{"data": {"me":[
2186+
{"pname":"nameJ","pred1":"C"}]}}`,
2187+
},
2188+
{14, 12, 5, true, `{"data": {"me":[]}}`},
2189+
{15, 12, 5, false, `{"data": {"me":[]}}`},
2190+
}
2191+
makeQuery := func(offset, first int32, desc, index bool) string {
2192+
pred1 := "pred1"
2193+
pred2 := "pred2"
2194+
if index {
2195+
pred1 = "index-pred1"
2196+
pred2 = "index-pred2"
2197+
}
2198+
order := ",orderasc: "
2199+
if desc {
2200+
order = ",orderdesc: "
2201+
}
2202+
q := "me(func: uid(61, 62, 63, 64, 65, 66, 67, 68, 69, 70), orderasc: " + pred1 +
2203+
order + pred2
2204+
if offset != -1 {
2205+
q += fmt.Sprintf(", offset: %d", offset)
2206+
}
2207+
if first != -1 {
2208+
q += fmt.Sprintf(", first: %d", first)
2209+
}
2210+
query := "{" + q + ") { pname pred1:" + pred1 + " pred2:" + pred2 + " } }"
2211+
return processQueryNoErr(t, query)
2212+
}
2213+
2214+
for _, tc := range tests {
2215+
// Case of sort with Index.
2216+
actual := makeQuery(tc.offset, tc.first, tc.desc, true)
2217+
require.JSONEqf(t, tc.result, actual, "Failed on index-testcase: %d\n", tc.index)
2218+
2219+
// Case of sort without index
2220+
actual = makeQuery(tc.offset, tc.first, tc.desc, false)
2221+
require.JSONEqf(t, tc.result, actual, "Failed on testcase: %d\n", tc.index)
2222+
}
2223+
}
2224+
20712225
func TestMultiSortPaginateWithOffset(t *testing.T) {
20722226
t.Parallel()
20732227
tests := []struct {

‎systest/mutations_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -599,16 +599,16 @@ func SortFacetsReturnNil(t *testing.T, c *dgo.Dgraph) {
599599
{
600600
"name":"Michael",
601601
"friend":[
602-
{
603-
"name":"Charlie"
604-
},
605602
{
606603
"name":"Alice",
607604
"friend|since":"2014-01-02T00:00:00Z"
608605
},
609606
{
610607
"name":"Sang Hyun",
611608
"friend|since":"2012-01-02T00:00:00Z"
609+
},
610+
{
611+
"name":"Charlie"
612612
}
613613
]
614614
}

‎types/sort.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,18 @@ func (s byValue) Less(i, j int) bool {
5959
return false
6060
}
6161
for vidx := range first {
62-
// Null value is considered greatest hence comes at first place while doing descending sort
63-
// and at last place while doing ascending sort.
64-
if first[vidx].Value == nil {
62+
// Null values are appended at the end of the sort result for both ascending and descending.
63+
// If both first and second has nil values, then maintain the order by UID.
64+
if first[vidx].Value == nil && second[vidx].Value == nil {
6565
return s.desc[vidx]
6666
}
6767

68+
if first[vidx].Value == nil {
69+
return false
70+
}
71+
6872
if second[vidx].Value == nil {
69-
return !s.desc[vidx]
73+
return true
7074
}
7175

7276
// We have to look at next value to decide.

‎worker/sort.go

+6
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,12 @@ BUCKETS:
334334
remainingCount := int(ts.Count) - len(r.UidMatrix[i].Uids)
335335
canAppend := x.Min(uint64(remainingCount), uint64(len(nullNodes)))
336336
r.UidMatrix[i].Uids = append(r.UidMatrix[i].Uids, nullNodes[:canAppend]...)
337+
338+
// The value list also need to contain null values for the appended uids.
339+
if len(ts.Order) > 1 {
340+
nullVals := make([]types.Val, canAppend)
341+
values[i] = append(values[i], nullVals...)
342+
}
337343
}
338344

339345
select {

0 commit comments

Comments
 (0)