From 8835004a4c1aea9556029281d10a8647422d9d26 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Thu, 6 Aug 2020 23:18:03 +0300 Subject: [PATCH] app/vmselect: properly handle PromQL queries like `scalar1 < metric < scalar2` like Prometheus does This fixes some cases from https://promlabs.com/promql-compliance-test-results-victoriametrics/ --- app/vmselect/promql/exec.go | 40 +++++++++++++++++ app/vmselect/promql/exec_test.go | 45 +++++++++++++++++++ go.mod | 2 +- go.sum | 4 +- .../VictoriaMetrics/metricsql/utils.go | 26 +++++++++++ vendor/modules.txt | 2 +- 6 files changed, 115 insertions(+), 4 deletions(-) diff --git a/app/vmselect/promql/exec.go b/app/vmselect/promql/exec.go index 2115b70d6..84e9b163f 100644 --- a/app/vmselect/promql/exec.go +++ b/app/vmselect/promql/exec.go @@ -133,10 +133,50 @@ func removeNaNs(tss []*timeseries) []*timeseries { return rvs } +func adjustCmpOps(e metricsql.Expr) metricsql.Expr { + metricsql.VisitAll(e, func(expr metricsql.Expr) { + be, ok := expr.(*metricsql.BinaryOpExpr) + if !ok { + return + } + if !metricsql.IsBinaryOpCmp(be.Op) { + return + } + if _, ok := be.Left.(*metricsql.NumberExpr); !ok { + return + } + // Convert 'num cmpOp query' expression to `query reverseCmpOp num` expression + // like Prometheus does. For isntance, `0.5 < foo` must be converted to `foo > 0.5` + // in order to return valid values for `foo` that are bigger than 0.5. + be.Right, be.Left = be.Left, be.Right + be.Op = getReverseCmpOp(be.Op) + }) + return e +} + +func getReverseCmpOp(op string) string { + switch op { + case ">": + return "<" + case "<": + return ">" + case ">=": + return "<=" + case "<=": + return ">=" + default: + // there is no need in changing `==` and `!=`. + return op + } +} + func parsePromQLWithCache(q string) (metricsql.Expr, error) { pcv := parseCacheV.Get(q) if pcv == nil { e, err := metricsql.Parse(q) + if err == nil { + e = adjustCmpOps(e) + } pcv = &parseCacheValue{ e: e, err: err, diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 949e28ba0..1bb91e2d6 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -1723,6 +1723,51 @@ func TestExecSuccess(t *testing.T) { resultExpected := []netstorage.Result{r1, r2} f(q, resultExpected) }) + t.Run(`scalar < time()`, func(t *testing.T) { + t.Parallel() + q := `123 < time()` + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{1000, 1200, 1400, 1600, 1800, 2000}, + Timestamps: timestampsExpected, + } + resultExpected := []netstorage.Result{r} + f(q, resultExpected) + }) + t.Run(`time() > scalar`, func(t *testing.T) { + t.Parallel() + q := `time() > 123` + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{1000, 1200, 1400, 1600, 1800, 2000}, + Timestamps: timestampsExpected, + } + resultExpected := []netstorage.Result{r} + f(q, resultExpected) + }) + t.Run(`scalar > time()`, func(t *testing.T) { + t.Parallel() + q := `123 > time()` + resultExpected := []netstorage.Result{} + f(q, resultExpected) + }) + t.Run(`time() < scalar`, func(t *testing.T) { + t.Parallel() + q := `time() < 123` + resultExpected := []netstorage.Result{} + f(q, resultExpected) + }) + t.Run(`scalar1 < time() < scalar2`, func(t *testing.T) { + t.Parallel() + q := `1300 < time() < 1700` + r := netstorage.Result{ + MetricName: metricNameExpected, + Values: []float64{nan, nan, 1400, 1600, nan, nan}, + Timestamps: timestampsExpected, + } + resultExpected := []netstorage.Result{r} + f(q, resultExpected) + }) t.Run(`a cmp scalar (leave MetricGroup)`, func(t *testing.T) { t.Parallel() q := `sort_desc(( diff --git a/go.mod b/go.mod index b3db6650c..33c65acc6 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( // like https://github.com/valyala/fasthttp/commit/996610f021ff45fdc98c2ce7884d5fa4e7f9199b github.com/VictoriaMetrics/fasthttp v1.0.4 github.com/VictoriaMetrics/metrics v1.12.2 - github.com/VictoriaMetrics/metricsql v0.3.0 + github.com/VictoriaMetrics/metricsql v0.4.0 github.com/aws/aws-sdk-go v1.33.19 github.com/cespare/xxhash/v2 v2.1.1 github.com/golang/snappy v0.0.1 diff --git a/go.sum b/go.sum index bbf1e4784..2ba24cf94 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,8 @@ github.com/VictoriaMetrics/fasthttp v1.0.4 h1:Aw6UqmPc0v5PunYOpiiyf4hk5B9PTMswdT github.com/VictoriaMetrics/fasthttp v1.0.4/go.mod h1:m5wCmg1dJN6s1B/lp8/dcKrnnM2l3DWB2eAGZGCTK3g= github.com/VictoriaMetrics/metrics v1.12.2 h1:SG8iAmqavDNuh7GIdHPoGHUhDL23KeKfvSZSozucNeA= github.com/VictoriaMetrics/metrics v1.12.2/go.mod h1:Z1tSfPfngDn12bTfZSCqArT3OPY3u88J12hSoOhuiRE= -github.com/VictoriaMetrics/metricsql v0.3.0 h1:610HmpZixn1KiQhV+/BNK/OAHE5M+8Fiy8LtI/xEguo= -github.com/VictoriaMetrics/metricsql v0.3.0/go.mod h1:ylO7YITho/Iw6P71oEaGyHbO94bGoGtzWfLGqFhMIg8= +github.com/VictoriaMetrics/metricsql v0.4.0 h1:1CMGiUzOgR83Y+hVS83/DaHYLaR6PFw1XWcvQnHn2V8= +github.com/VictoriaMetrics/metricsql v0.4.0/go.mod h1:ylO7YITho/Iw6P71oEaGyHbO94bGoGtzWfLGqFhMIg8= github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156 h1:eMwmnE/GDgah4HI848JfFxHt+iPb26b4zyfspmqY0/8= github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156/go.mod h1:Cb/ax3seSYIx7SuZdm2G2xzfwmv3TPSk2ucNfQESPXM= github.com/andybalholm/brotli v1.0.0/go.mod h1:loMXtMfwqflxFJPmdbJO0a3KNoPuLBgiu3qAvBg8x/Y= diff --git a/vendor/github.com/VictoriaMetrics/metricsql/utils.go b/vendor/github.com/VictoriaMetrics/metricsql/utils.go index 3e3fa8d6e..11828f39f 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/utils.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/utils.go @@ -10,3 +10,29 @@ func ExpandWithExprs(q string) (string, error) { buf := e.AppendString(nil) return string(buf), nil } + +// VisitAll recursively calls f for all the Expr children in e. +// +// It visits leaf children at first and then visits parent nodes. +// It is safe modifying expr in f. +func VisitAll(e Expr, f func(expr Expr)) { + switch expr := e.(type) { + case *BinaryOpExpr: + VisitAll(expr.Left, f) + VisitAll(expr.Right, f) + VisitAll(&expr.GroupModifier, f) + VisitAll(&expr.JoinModifier, f) + case *FuncExpr: + for _, arg := range expr.Args { + VisitAll(arg, f) + } + case *AggrFuncExpr: + for _, arg := range expr.Args { + VisitAll(arg, f) + } + VisitAll(&expr.Modifier, f) + case *RollupExpr: + VisitAll(expr.Expr, f) + } + f(e) +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 74ade73c3..fd0a3f3e6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -16,7 +16,7 @@ github.com/VictoriaMetrics/fasthttp/fasthttputil github.com/VictoriaMetrics/fasthttp/stackless # github.com/VictoriaMetrics/metrics v1.12.2 github.com/VictoriaMetrics/metrics -# github.com/VictoriaMetrics/metricsql v0.3.0 +# github.com/VictoriaMetrics/metricsql v0.4.0 github.com/VictoriaMetrics/metricsql github.com/VictoriaMetrics/metricsql/binaryop # github.com/aws/aws-sdk-go v1.33.19