From b047feeb8b87d48bf751db4876cc6b2af5d04ba0 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Fri, 16 Jul 2021 01:43:01 +0300 Subject: [PATCH] app/vmselect/promql: properly handle `(a op b) default N` if `(a op b)` returns NaN series The result should be a series with `N` values and `a op b` labels. Previously such series has been removed from the result. --- app/vmselect/promql/binary_op.go | 5 ++--- app/vmselect/promql/exec_test.go | 35 ++++++++++++++++++++++++-------- app/vmselect/promql/transform.go | 11 ++++++++-- docs/CHANGELOG.md | 2 ++ 4 files changed, 40 insertions(+), 13 deletions(-) diff --git a/app/vmselect/promql/binary_op.go b/app/vmselect/promql/binary_op.go index b103b20756..512d326636 100644 --- a/app/vmselect/promql/binary_op.go +++ b/app/vmselect/promql/binary_op.go @@ -100,9 +100,8 @@ func newBinaryOpFunc(bf func(left, right float64, isBool bool) float64) binaryOp dstValues[j] = bf(a, b, isBool) } } - // Optimization: remove time series containing only NaNs. - // This is quite common after applying filters like `q > 0`. - dst = removeNaNs(dst) + // Do not remove time series containing only NaNs, since then the `(foo op bar) default N` + // won't work as expected if `(foo op bar)` results to NaN series. return dst, nil } } diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 78466ba6f9..d687f50a4f 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -1054,6 +1054,21 @@ func TestExecSuccess(t *testing.T) { resultExpected := []netstorage.Result{r} f(q, resultExpected) }) + t.Run("default_for_nan_series", func(t *testing.T) { + t.Parallel() + q := `label_set(0, "foo", "bar")/0 default 7` + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{7, 7, 7, 7, 7, 7}, + Timestamps: timestampsExpected, + } + r.MetricName.Tags = []storage.Tag{{ + Key: []byte("foo"), + Value: []byte("bar"), + }} + resultExpected := []netstorage.Result{r} + f(q, resultExpected) + }) t.Run(`alias()`, func(t *testing.T) { t.Parallel() q := `alias(time(), "foobar")` @@ -1701,10 +1716,12 @@ func TestExecSuccess(t *testing.T) { Values: []float64{1123.456, 1323.456, 1523.456, 1723.456, 1923.456, 2123.456}, Timestamps: timestampsExpected, } - r2.MetricName.Tags = []storage.Tag{{ - Key: []byte("foo"), - Value: []byte("123.456"), - }} + r2.MetricName.Tags = []storage.Tag{ + { + Key: []byte("foo"), + Value: []byte("123.456"), + }, + } resultExpected := []netstorage.Result{r1, r2} f(q, resultExpected) }) @@ -6961,19 +6978,21 @@ func testMetricNamesEqual(t *testing.T, mn, mnExpected *storage.MetricName, pos t.Fatalf(`unexpected projectID; got %d; want %d`, mn.ProjectID, mnExpected.ProjectID) } if string(mn.MetricGroup) != string(mnExpected.MetricGroup) { - t.Fatalf(`unexpected MetricGroup at #%d; got %q; want %q`, pos, mn.MetricGroup, mnExpected.MetricGroup) + t.Fatalf(`unexpected MetricGroup at #%d; got %q; want %q; metricGot=%s, metricExpected=%s`, + pos, mn.MetricGroup, mnExpected.MetricGroup, mn.String(), mnExpected.String()) } if len(mn.Tags) != len(mnExpected.Tags) { - t.Fatalf(`unexpected tags count at #%d; got %d; want %d`, pos, len(mn.Tags), len(mnExpected.Tags)) + t.Fatalf(`unexpected tags count at #%d; got %d; want %d; metricGot=%s, metricExpected=%s`, pos, len(mn.Tags), len(mnExpected.Tags), mn.String(), mnExpected.String()) } for i := range mn.Tags { tag := &mn.Tags[i] tagExpected := &mnExpected.Tags[i] if string(tag.Key) != string(tagExpected.Key) { - t.Fatalf(`unexpected tag key at #%d,%d; got %q; want %q`, pos, i, tag.Key, tagExpected.Key) + t.Fatalf(`unexpected tag key at #%d,%d; got %q; want %q; metricGot=%s, metricExpected=%s`, pos, i, tag.Key, tagExpected.Key, mn.String(), mnExpected.String()) } if string(tag.Value) != string(tagExpected.Value) { - t.Fatalf(`unexpected tag value for key %q at #%d,%d; got %q; want %q`, tag.Key, pos, i, tag.Value, tagExpected.Value) + t.Fatalf(`unexpected tag value for key %q at #%d,%d; got %q; want %q; metricGot=%s, metricExpected=%s`, + tag.Key, pos, i, tag.Value, tagExpected.Value, mn.String(), mnExpected.String()) } } } diff --git a/app/vmselect/promql/transform.go b/app/vmselect/promql/transform.go index 68a75fead3..9ef2fefd76 100644 --- a/app/vmselect/promql/transform.go +++ b/app/vmselect/promql/transform.go @@ -1823,8 +1823,15 @@ func newTransformFuncSort(isDesc bool) transformFunc { b := rvs[j].Values n := len(a) - 1 for n >= 0 { - if !math.IsNaN(a[n]) && !math.IsNaN(b[n]) && a[n] != b[n] { - break + if !math.IsNaN(a[n]) { + if math.IsNaN(b[n]) { + return false + } + if a[n] != b[n] { + break + } + } else if !math.IsNaN(b[n]) { + return true } n-- } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 58b481330d..6dcb559439 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -8,6 +8,8 @@ sort: 15 * FEATURE: add `-search.maxSamplesPerSeries` command-line flag for limiting the number of raw samples a single query could process per each time series. This option can prevent from out of memory errors when a query processes tens of millions of raw samples per series. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/1067). +* BUGFIX: return series with `a op b` labels and `N` values for `(a op b) default N` if `(a op b)` returns series with all NaN values. Previously such series were removed. + ## [v1.63.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.63.0)