From 490c70a95883a4f469b28ae303a73bc1acf04468 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 2 Dec 2020 12:08:47 +0200 Subject: [PATCH] app/vmselect: return `metric` values from `time() cmp_op metric` query when `cmp_op` comparison is true This aligns MetricsQL behavior to Prometheus' one. The issue has been identified at https://promlabs.com/promql-compliance-test-results/2020-12-01/victoriametrics/ --- app/vmselect/promql/exec.go | 20 ++++++++++++++++++-- docs/CHANGELOG.md | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/vmselect/promql/exec.go b/app/vmselect/promql/exec.go index 79df6bb841..8acb8a77e6 100644 --- a/app/vmselect/promql/exec.go +++ b/app/vmselect/promql/exec.go @@ -149,11 +149,11 @@ func adjustCmpOps(e metricsql.Expr) metricsql.Expr { if !metricsql.IsBinaryOpCmp(be.Op) { return } - if _, ok := be.Left.(*metricsql.NumberExpr); !ok { + if isNumberExpr(be.Right) || !isScalarExpr(be.Left) { 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` + // like Prometheus does. For instance, `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) @@ -161,6 +161,22 @@ func adjustCmpOps(e metricsql.Expr) metricsql.Expr { return e } +func isNumberExpr(e metricsql.Expr) bool { + _, ok := e.(*metricsql.NumberExpr) + return ok +} + +func isScalarExpr(e metricsql.Expr) bool { + if isNumberExpr(e) { + return true + } + if fe, ok := e.(*metricsql.FuncExpr); ok { + // time() returns scalar in PromQL - see https://prometheus.io/docs/prometheus/latest/querying/functions/#time + return strings.ToLower(fe.Name) == "time" + } + return false +} + func getReverseCmpOp(op string) string { switch op { case ">": diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9a5989648d..fc9d2b3aef 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,6 +7,7 @@ * BUGFIX: return `nan` for `a >bool b` query when `a` equals to `nan` like Prometheus does. Previously `0` was returned in this case. This applies to any comparison operation with `bool` modifier. See [these docs](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators) for details. * BUGFIX: properly parse hex numbers in MetricsQL. Previously hex numbers with non-decimal digits such as `0x3b` couldn't be parsed. +* BUGFIX: Handle `time() cmp_op metric` like Prometheus does - i.e. return `metric` value if `cmp_op` comparison is true. Previously `time()` value was returned. # [v1.48.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.48.0)