From b5da47bfaf2f573853645030bcf1ff0db331d31a Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Mon, 18 Jul 2022 14:23:07 +0300 Subject: [PATCH] app/vmselect/promql: properly return `q1` series from `q1 ifnot q2` when `q2` returns nothing --- app/vmselect/promql/binary_op.go | 182 ++++++++++++++++++++----------- app/vmselect/promql/exec_test.go | 20 +++- docs/CHANGELOG.md | 1 + 3 files changed, 140 insertions(+), 63 deletions(-) diff --git a/app/vmselect/promql/binary_op.go b/app/vmselect/promql/binary_op.go index 699f54d18..8e78664ff 100644 --- a/app/vmselect/promql/binary_op.go +++ b/app/vmselect/promql/binary_op.go @@ -36,9 +36,9 @@ var binaryOpFuncs = map[string]binaryOpFunc{ "unless": binaryOpUnless, // New ops - "if": newBinaryOpArithFunc(binaryop.If), - "ifnot": newBinaryOpArithFunc(binaryop.Ifnot), - "default": newBinaryOpArithFunc(binaryop.Default), + "if": binaryOpIf, + "ifnot": binaryOpIfnot, + "default": binaryOpDefault, } func getBinaryOpFunc(op string) binaryOpFunc { @@ -86,17 +86,6 @@ func newBinaryOpFunc(bf func(left, right float64, isBool bool) float64) binaryOp right := bfa.right op := bfa.be.Op switch true { - case op == "ifnot": - left = removeEmptySeries(left) - // Do not remove empty series on the right side, - // so the left-side series could be matched against them. - case op == "default": - // Do not remove empty series on the left and the right side, - // since this may lead to missing result: - // - if empty time series are removed on the left side, - // then they won't be substituted by time series from the right side. - // - if empty time series are removed on the right side, - // then this may result in missing time series from the left side. case metricsql.IsBinaryOpCmp(op): // Do not remove empty series for comparison operations, // since this may lead to missing result. @@ -136,7 +125,7 @@ func newBinaryOpFunc(bf func(left, right float64, isBool bool) float64) binaryOp func adjustBinaryOpTags(be *metricsql.BinaryOpExpr, left, right []*timeseries) ([]*timeseries, []*timeseries, []*timeseries, error) { if len(be.GroupModifier.Op) == 0 && len(be.JoinModifier.Op) == 0 { - if isScalar(left) && be.Op != "default" && be.Op != "if" && be.Op != "ifnot" { + if isScalar(left) { // Fast path: `scalar op vector` rvsLeft := make([]*timeseries, len(right)) tsLeft := left[0] @@ -324,14 +313,23 @@ func resetMetricGroupIfRequired(be *metricsql.BinaryOpExpr, ts *timeseries) { // Do not reset MetricGroup for non-boolean `compare` binary ops like Prometheus does. return } - switch be.Op { - case "default", "if", "ifnot": - // Do not reset MetricGroup for these ops. - return - } ts.MetricName.ResetMetricGroup() } +func binaryOpIf(bfa *binaryOpFuncArg) ([]*timeseries, error) { + mLeft, mRight := createTimeseriesMapByTagSet(bfa.be, bfa.left, bfa.right) + var rvs []*timeseries + for k, tssLeft := range mLeft { + tssRight := seriesByKey(mRight, k) + if tssRight == nil { + continue + } + tssLeft = addRightNaNsToLeft(tssLeft, tssRight) + rvs = append(rvs, tssLeft...) + } + return rvs, nil +} + func binaryOpAnd(bfa *binaryOpFuncArg) ([]*timeseries, error) { mLeft, mRight := createTimeseriesMapByTagSet(bfa.be, bfa.left, bfa.right) var rvs []*timeseries @@ -340,24 +338,47 @@ func binaryOpAnd(bfa *binaryOpFuncArg) ([]*timeseries, error) { if tssLeft == nil { continue } - // Add gaps to tssLeft if there are gaps at tssRight. - for _, tsLeft := range tssLeft { - valuesLeft := tsLeft.Values - for i := range valuesLeft { - hasValue := false - for _, tsRight := range tssRight { - if !math.IsNaN(tsRight.Values[i]) { - hasValue = true - break - } - } - if !hasValue { - valuesLeft[i] = nan + tssLeft = addRightNaNsToLeft(tssLeft, tssRight) + rvs = append(rvs, tssLeft...) + } + return rvs, nil +} + +func addRightNaNsToLeft(tssLeft, tssRight []*timeseries) []*timeseries { + for _, tsLeft := range tssLeft { + valuesLeft := tsLeft.Values + for i := range valuesLeft { + hasValue := false + for _, tsRight := range tssRight { + if !math.IsNaN(tsRight.Values[i]) { + hasValue = true + break } } + if !hasValue { + valuesLeft[i] = nan + } } - tssLeft = removeEmptySeries(tssLeft) + } + return removeEmptySeries(tssLeft) +} + +func binaryOpDefault(bfa *binaryOpFuncArg) ([]*timeseries, error) { + mLeft, mRight := createTimeseriesMapByTagSet(bfa.be, bfa.left, bfa.right) + var rvs []*timeseries + if len(mLeft) == 0 { + for _, tss := range mRight { + rvs = append(rvs, tss...) + } + return rvs, nil + } + for k, tssLeft := range mLeft { rvs = append(rvs, tssLeft...) + tssRight := seriesByKey(mRight, k) + if tssRight == nil { + continue + } + fillLeftNaNsWithRightValues(tssLeft, tssRight) } return rvs, nil } @@ -374,24 +395,43 @@ func binaryOpOr(bfa *binaryOpFuncArg) ([]*timeseries, error) { rvs = append(rvs, tssRight...) continue } - // Fill gaps in tssLeft with values from tssRight as Prometheus does. - // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/552 - for _, tsLeft := range tssLeft { - valuesLeft := tsLeft.Values - for i, v := range valuesLeft { - if !math.IsNaN(v) { - continue - } - for _, tsRight := range tssRight { - vRight := tsRight.Values[i] - if !math.IsNaN(vRight) { - valuesLeft[i] = vRight - break - } + fillLeftNaNsWithRightValues(tssLeft, tssRight) + } + return rvs, nil +} + +func fillLeftNaNsWithRightValues(tssLeft, tssRight []*timeseries) { + // Fill gaps in tssLeft with values from tssRight as Prometheus does. + // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/552 + for _, tsLeft := range tssLeft { + valuesLeft := tsLeft.Values + for i, v := range valuesLeft { + if !math.IsNaN(v) { + continue + } + for _, tsRight := range tssRight { + vRight := tsRight.Values[i] + if !math.IsNaN(vRight) { + valuesLeft[i] = vRight + break } } } } +} + +func binaryOpIfnot(bfa *binaryOpFuncArg) ([]*timeseries, error) { + mLeft, mRight := createTimeseriesMapByTagSet(bfa.be, bfa.left, bfa.right) + var rvs []*timeseries + for k, tssLeft := range mLeft { + tssRight := seriesByKey(mRight, k) + if tssRight == nil { + rvs = append(rvs, tssLeft...) + continue + } + tssLeft = addLeftNaNsIfNoRightNaNs(tssLeft, tssRight) + rvs = append(rvs, tssLeft...) + } return rvs, nil } @@ -404,24 +444,44 @@ func binaryOpUnless(bfa *binaryOpFuncArg) ([]*timeseries, error) { rvs = append(rvs, tssLeft...) continue } - // Add gaps to tssLeft if the are no gaps at tssRight. - for _, tsLeft := range tssLeft { - valuesLeft := tsLeft.Values - for i := range valuesLeft { - for _, tsRight := range tssRight { - if !math.IsNaN(tsRight.Values[i]) { - valuesLeft[i] = nan - break - } - } - } - } - tssLeft = removeEmptySeries(tssLeft) + tssLeft = addLeftNaNsIfNoRightNaNs(tssLeft, tssRight) rvs = append(rvs, tssLeft...) } return rvs, nil } +func addLeftNaNsIfNoRightNaNs(tssLeft, tssRight []*timeseries) []*timeseries { + for _, tsLeft := range tssLeft { + valuesLeft := tsLeft.Values + for i := range valuesLeft { + for _, tsRight := range tssRight { + if !math.IsNaN(tsRight.Values[i]) { + valuesLeft[i] = nan + break + } + } + } + } + return removeEmptySeries(tssLeft) +} + +func seriesByKey(m map[string][]*timeseries, key string) []*timeseries { + tss := m[key] + if tss != nil { + return tss + } + if len(m) != 1 { + return nil + } + for _, tss := range m { + if isScalar(tss) { + return tss + } + return nil + } + return nil +} + func createTimeseriesMapByTagSet(be *metricsql.BinaryOpExpr, left, right []*timeseries) (map[string][]*timeseries, map[string][]*timeseries) { groupTags := be.GroupModifier.Args groupOp := strings.ToLower(be.GroupModifier.Op) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 350c1e35f..4727d98cb 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -2803,7 +2803,12 @@ func TestExecSuccess(t *testing.T) { t.Run(`scalar default vector1`, func(t *testing.T) { t.Parallel() q := `time() > 1400 default label_set(123, "foo", "bar")` - resultExpected := []netstorage.Result{} + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{nan, nan, nan, 1600, 1800, 2000}, + Timestamps: timestampsExpected, + } + resultExpected := []netstorage.Result{r} f(q, resultExpected) }) t.Run(`scalar default vector2`, func(t *testing.T) { @@ -6092,7 +6097,18 @@ func TestExecSuccess(t *testing.T) { t.Run(`ifnot-no-matching-timeseries`, func(t *testing.T) { t.Parallel() q := `label_set(time(), "foo", "bar") ifnot label_set(time() > 1400, "x", "y")` - resultExpected := []netstorage.Result{} + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{1000, 1200, 1400, 1600, 1800, 2000}, + Timestamps: timestampsExpected, + } + r.MetricName.Tags = []storage.Tag{ + { + Key: []byte("foo"), + Value: []byte("bar"), + }, + } + resultExpected := []netstorage.Result{r} f(q, resultExpected) }) t.Run(`quantile(-2)`, func(t *testing.T) { diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 39b2ec815..4d687d33d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -16,6 +16,7 @@ The following tip changes can be tested by building VictoriaMetrics components f ## tip * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly assume role with AWS ECS credentials. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2875). Thanks to @transacid for [the fix](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/2876). +* BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): return series from `q1` if `q2` doesn't return matching time series in the query `q1 ifnot q2`. Previously series from `q1` weren't returned in this case. ## [v1.79.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.79.0)