Repository: VictoriaMetrics/VictoriaMetrics
Pull request #5509 (2023-12-21 08:35:50)

FIX bottomk doesn't return any data when there are no time range overlap between timeseries
try to fix [#5506]

xiaozongyang: Hi @valyala @hagen1778 @f41gh7, would you mind checking this issue and review this MR?
xiaozongyang: This fix doesn't work
xiaozongyang: sure, I will fix the lint errors in next commit
xiaozongyang: Hi @hagen1778, these lint errors have been fixed, but there is a fatal error about unit test of `github.com/VictoriaMetrics/VictoriaMetrics/lib/storage.go`. I've no idea about fixing this error, would you mind helping check whether it's cause by this PR or not? Or just ignore this error temperately?
If you have some suggestions to fix this error, I'd like to try to fix it !
cc @valyala 
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/5509?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`910a39a`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/910a39ad7227c77fa1922e0d55222d10eef5a563?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.18% compared to head [(`b76cafa`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/5509?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.18%.
> Report is 5 commits behind head on master.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #5509      +/-   ##
==========================================
- Coverage   60.18%   60.18%   -0.01%     
==========================================
  Files         401      401              
  Lines       75494    75501       +7     
==========================================
+ Hits        45436    45438       +2     
- Misses      27525    27530       +5     
  Partials     2533     2533              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/5509?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).

xiaozongyang: ping @hagen1778, lint and test errors have been fixed already, would you mind reviewing this PR once more?
xiaozongyang: @hagen1778 @valyala Merry Christmas  you guys, and have a nice holiday. It's ok to review this PR later.

Patch:
diff --git a/app/vmselect/promql/aggr.go b/app/vmselect/promql/aggr.go
index 943be4210..47d4f4cc9 100644
--- a/app/vmselect/promql/aggr.go
+++ b/app/vmselect/promql/aggr.go
@@ -649,15 +649,16 @@ func newAggrFuncTopK(isReverse bool) aggrFunc {
 		if err != nil {
 			return nil, err
 		}
+		lt := lessWithNaNs
+		if isReverse {
+			lt = lessWithNaNsReversed
+		}
 		afe := func(tss []*timeseries, modififer *metricsql.ModifierExpr) []*timeseries {
 			for n := range tss[0].Values {
 				sort.Slice(tss, func(i, j int) bool {
 					a := tss[i].Values[n]
 					b := tss[j].Values[n]
-					if isReverse {
-						a, b = b, a
-					}
-					return lessWithNaNs(a, b)
+					return lt(a, b)
 				})
 				fillNaNsAtIdx(n, ks[n], tss)
 			}
@@ -710,13 +711,12 @@ func getRangeTopKTimeseries(tss []*timeseries, modifier *metricsql.ModifierExpr,
 			value: value,
 		}
 	}
+	lt := lessWithNaNs
+	if isReverse {
+		lt = lessWithNaNsReversed
+	}
 	sort.Slice(maxs, func(i, j int) bool {
-		a := maxs[i].value
-		b := maxs[j].value
-		if isReverse {
-			a, b = b, a
-		}
-		return lessWithNaNs(a, b)
+		return lt(maxs[i].value, maxs[j].value)
 	})
 	for i := range maxs {
 		tss[i] = maxs[i].ts
@@ -1259,6 +1259,13 @@ func lessWithNaNs(a, b float64) bool {
 	return a < b
 }
 
+func lessWithNaNsReversed(a, b float64) bool {
+	if math.IsNaN(a) {
+		return true
+	}
+	return a > b
+}
+
 func floatToIntBounded(f float64) int {
 	if f > math.MaxInt {
 		return math.MaxInt
diff --git a/lib/filestream/filestream.go b/lib/filestream/filestream.go
index 339f72855..a6eebc143 100644
--- a/lib/filestream/filestream.go
+++ b/lib/filestream/filestream.go
@@ -334,6 +334,6 @@ var bwPool sync.Pool
 
 type streamTracker struct {
 	fd     uintptr
-	offset uint64
-	length uint64
+	offset uint64 // nolint
+	length uint64 // nolint
 }
diff --git a/lib/filestream/filestream_darwin.go b/lib/filestream/filestream_darwin.go
index 040660083..e70c9ba21 100644
--- a/lib/filestream/filestream_darwin.go
+++ b/lib/filestream/filestream_darwin.go
@@ -1,6 +1,6 @@
 package filestream
 
-func (st *streamTracker) adviseDontNeed(n int, fdatasync bool) error {
+func (st *streamTracker) adviseDontNeed(n int, fdatasync bool) error { // nolint
 	return nil
 }
 
diff --git a/lib/fs/fadvise_darwin.go b/lib/fs/fadvise_darwin.go
index 73cfe81a7..c65a3f435 100644
--- a/lib/fs/fadvise_darwin.go
+++ b/lib/fs/fadvise_darwin.go
@@ -4,7 +4,7 @@ import (
 	"os"
 )
 
-func fadviseSequentialRead(f *os.File, prefetch bool) error {
+func fadviseSequentialRead(f *os.File, prefetch bool) error { // nolint
 	// TODO: implement this properly
 	return nil
 }


Test patch:
diff --git a/app/vmselect/promql/aggr_test.go b/app/vmselect/promql/aggr_test.go
index a8c58883f..bef5a6c04 100644
--- a/app/vmselect/promql/aggr_test.go
+++ b/app/vmselect/promql/aggr_test.go
@@ -1,8 +1,12 @@
 package promql
 
 import (
+	"log"
 	"math"
+	"reflect"
 	"testing"
+
+	"github.com/VictoriaMetrics/metricsql"
 )
 
 func TestModeNoNaNs(t *testing.T) {
@@ -34,3 +38,268 @@ func TestModeNoNaNs(t *testing.T) {
 	f(1, []float64{2, 3, 3, 4, 4}, 3)
 	f(1, []float64{4, 3, 2, 3, 4}, 3)
 }
+
+func TestLessWithNaNs(t *testing.T) {
+	f := func(a, b float64, expectedResult bool) {
+		t.Helper()
+		result := lessWithNaNs(a, b)
+		if result != expectedResult {
+			t.Fatalf("unexpected result; got %v; want %v", result, expectedResult)
+		}
+	}
+	f(nan, nan, false)
+	f(nan, 1, true)
+	f(1, nan, false)
+	f(1, 2, true)
+	f(2, 1, false)
+	f(1, 1, false)
+}
+
+func TestLessWithNaNsReversed(t *testing.T) {
+	f := func(a, b float64, expectedResult bool) {
+		t.Helper()
+		result := lessWithNaNsReversed(a, b)
+		if result != expectedResult {
+			t.Fatalf("unexpected result; got %v; want %v", result, expectedResult)
+		}
+	}
+	f(nan, nan, true)
+	f(nan, 1, true)
+	f(1, nan, false)
+	f(1, 2, false)
+	f(2, 1, true)
+	f(1, 1, false)
+}
+
+func TestTopK(t *testing.T) {
+	f := func(all [][]*timeseries, expected []*timeseries, k int, reversed bool) {
+		t.Helper()
+		topKFunc := newAggrFuncTopK(reversed)
+		actual, err := topKFunc(&aggrFuncArg{
+			args: all,
+			ae: &metricsql.AggrFuncExpr{
+				Limit:    1,
+				Modifier: metricsql.ModifierExpr{},
+			},
+			ec: nil,
+		})
+		if err != nil {
+			log.Fatalf("failed to call topK, err=%v", err)
+		}
+		for i := range actual {
+			if !eq(expected[i], actual[i]) {
+				t.Fatalf("unexpected result: i:%v got:\n%v; want:\t%v", i, actual[i], expected[i])
+			}
+		}
+	}
+
+	f(newTestSeries(), []*timeseries{
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{nan, nan, 3, 2, 1},
+		},
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{1, 2, 3, 4, 5},
+		},
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{2, 3, nan, nan, nan},
+		},
+	}, 2, true)
+	f(newTestSeries(), []*timeseries{
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{3, 4, 5, 6, 7},
+		},
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{nan, nan, 4, 5, 6},
+		},
+		{
+			Timestamps: []int64{1, 2, 3, 4, 5},
+			Values:     []float64{5, 4, nan, nan, nan},
+		},
+	}, 2, false)
+	f(newTestSeriesWithNaNsWithoutOverlap(), []*timeseries{
+		{
+			Values:     []float64{nan, nan, nan, 2, 1},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{nan, nan, 5, 6, 7},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{2, 3, 4, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{1, 2, nan, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+	}, 2, true)
+	f(newTestSeriesWithNaNsWithoutOverlap(), []*timeseries{
+		{
+			Values:     []float64{nan, nan, 5, 6, 7},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{nan, nan, 6, 2, 1},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{2, 3, nan, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{1, 2, nan, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+	}, 2, false)
+	f(newTestSeriesWithNaNsWithOverlap(), []*timeseries{
+		{
+			Values:     []float64{nan, nan, nan, 2, 1},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{nan, nan, nan, 6, 7},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{1, 2, 3, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{2, 3, 4, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+	}, 2, true)
+	f(newTestSeriesWithNaNsWithOverlap(), []*timeseries{
+		{
+			Values:     []float64{nan, nan, 5, 6, 7},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{nan, nan, 6, 2, 1},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{2, 3, nan, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+		{
+			Values:     []float64{1, 2, nan, nan, nan},
+			Timestamps: []int64{1, 2, 3, 4, 5},
+		},
+	}, 2, false)
+}
+
+func newTestSeries() [][]*timeseries {
+	return [][]*timeseries{
+		{
+			{
+				Values:     []float64{2, 2, 2, 2, 2},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+		{
+			{
+				Values:     []float64{1, 2, 3, 4, 5},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{2, 3, 4, 5, 6},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{5, 4, 3, 2, 1},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{3, 4, 5, 6, 7},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+	}
+}
+
+func newTestSeriesWithNaNsWithoutOverlap() [][]*timeseries {
+	return [][]*timeseries{
+		{
+			{
+				Values:     []float64{2, 2, 2, 2, 2},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+		{
+			{
+				Values:     []float64{1, 2, nan, nan, nan},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{2, 3, 4, nan, nan},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{nan, nan, 6, 2, 1},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{nan, nan, 5, 6, 7},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+	}
+}
+
+func newTestSeriesWithNaNsWithOverlap() [][]*timeseries {
+	return [][]*timeseries{
+		{
+			{
+				Values:     []float64{2, 2, 2, 2, 2},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+		{
+			{
+				Values:     []float64{1, 2, 3, nan, nan},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{2, 3, 4, nan, nan},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{nan, nan, 6, 2, 1},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+			{
+				Values:     []float64{nan, nan, 5, 6, 7},
+				Timestamps: []int64{1, 2, 3, 4, 5},
+			},
+		},
+	}
+}
+
+func eq(a, b *timeseries) bool {
+	if !reflect.DeepEqual(a.Timestamps, b.Timestamps) {
+		return false
+	}
+	for i := range a.Values {
+		if !eqWithNan(a.Values[i], b.Values[i]) {
+			return false
+		}
+	}
+	return true
+}
+
+func eqWithNan(a, b float64) bool {
+	if math.IsNaN(a) && math.IsNaN(b) {
+		return true
+	}
+	if math.IsNaN(a) || math.IsNaN(b) {
+		return false
+	}
+	return a == b
+}