From 9cbe13931e3c3d79932c83f55ffb5d52413dbe22 Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Sun, 22 Oct 2023 19:59:17 +0200 Subject: [PATCH 1/8] Bug: Fix LogQL `LiteralEvaluator` Binary Operation Repect the Promql following behaviour of Binary operations with vector vs scalar 1. `scalar` op `vector` - vector 2. `vector` op `scalar` - vector Signed-off-by: Kaviraj --- pkg/logql/bug_test.go | 94 ++++++++++++++++++++++++++++++++++++ pkg/logql/evaluator.go | 3 +- pkg/logql/evaluator_test.go | 14 +++--- pkg/logql/syntax/ast.go | 30 ++++++++---- pkg/logql/syntax/ast_test.go | 9 ++-- 5 files changed, 129 insertions(+), 21 deletions(-) create mode 100644 pkg/logql/bug_test.go diff --git a/pkg/logql/bug_test.go b/pkg/logql/bug_test.go new file mode 100644 index 0000000000000..7c6ba3075293d --- /dev/null +++ b/pkg/logql/bug_test.go @@ -0,0 +1,94 @@ +package logql + +import ( + "context" + "fmt" + "math" + "testing" + "time" + + "github.com/grafana/dskit/user" + "github.com/grafana/loki/pkg/logproto" + "github.com/stretchr/testify/require" + + "github.com/go-kit/log" +) + +func TestScalarVectorBinOp(t *testing.T) { + NoLimits = &fakeLimits{maxSeries: math.MaxInt32} + + qs1 := `sum(count_over_time({app="foo"}[1m])) > 3` + qs2 := `3 < sum(count_over_time({app="foo"}[1m]))` + + data := [][]logproto.Series{ + {newSeries(4, constant(70), `{app="foo",pool="foo"}`)}, + } + + fmt.Printf("%+v\n", data) + + params := []SelectSampleParams{ + {&logproto.SampleQueryRequest{Start: time.Unix(10, 0), End: time.Unix(70, 0), Selector: `sum(count_over_time({app="foo"}[1m]))`}}, + } + + querier := newQuerierRecorder(t, data, params) + + engine := NewEngine(EngineOpts{}, querier, NoLimits, log.NewNopLogger()) + start := time.Unix(70, 0) + end := time.Unix(70, 0) + direction := logproto.FORWARD + limit := uint32(0) + + q1 := engine.Query(LiteralParams{qs: qs1, start: start, end: end, direction: direction, limit: limit}) + res1, err := q1.Exec(user.InjectOrgID(context.Background(), "fake")) + if err != nil { + t.Fatal(err) + } + + q2 := engine.Query(LiteralParams{qs: qs2, start: start, end: end, direction: direction, limit: limit}) + res2, err := q2.Exec(user.InjectOrgID(context.Background(), "fake")) + if err != nil { + t.Fatal(err) + } + + require.Equal(t, res1.Data, res2.Data) + + // fmt.Println("Return Data type:", res.Data.Type()) + // fmt.Printf("%+v\n", res.Data) +} + +// func TestBugNotWorking(t *testing.T) { +// NoLimits = &fakeLimits{maxSeries: math.MaxInt32} + +// data := [][]logproto.Series{ +// {newSeries(4, constant(70), `{app="foo",pool="foo"}`)}, +// } + +// params := []SelectSampleParams{ +// {&logproto.SampleQueryRequest{Start: time.Unix(10, 0), End: time.Unix(70, 0), Selector: `sum(count_over_time({app="foo"}[1m]))`}}, +// } + +// querier := newQuerierRecorder(t, data, params) + +// engine := NewEngine(EngineOpts{}, querier, NoLimits, log.NewNopLogger()) +// start := time.Unix(70, 0) +// end := time.Unix(70, 0) +// direction := logproto.FORWARD +// limit := uint32(0) + +// q := engine.Query(LiteralParams{qs: qs, start: start, end: end, direction: direction, limit: limit}) +// res, err := q.Exec(user.InjectOrgID(context.Background(), "fake")) +// if err != nil { +// t.Fatal(err) +// } + +// fmt.Println("Return Data type:", res.Data.Type()) +// fmt.Printf("%+v\n", res.Data) +// } + +// func TestUnderstanding(t *testing.T) { +// data := [][]logproto.Series{ +// {newSeries(2, identity, `{app="foo"}`)}, +// } + +// fmt.Printf("%+v\n", data) +// } diff --git a/pkg/logql/evaluator.go b/pkg/logql/evaluator.go index 1e5a5f19768f1..07a056e4ffe58 100644 --- a/pkg/logql/evaluator.go +++ b/pkg/logql/evaluator.go @@ -809,7 +809,7 @@ func vectorBinop(op string, opts *syntax.BinOpOptions, lhs, rhs promql.Vector, l ls, rs = rs, ls } } - merged, err := syntax.MergeBinOp(op, ls, rs, filter, syntax.IsComparisonOperator(op)) + merged, err := syntax.MergeBinOp(op, ls, rs, false, filter, syntax.IsComparisonOperator(op)) if err != nil { return nil, err } @@ -973,6 +973,7 @@ func (e *LiteralStepEvaluator) Next() (bool, int64, StepResult) { e.op, left, right, + !e.inverted, !e.returnBool, syntax.IsComparisonOperator(e.op), ) diff --git a/pkg/logql/evaluator_test.go b/pkg/logql/evaluator_test.go index bc600d6707bf2..9f0ddebbf0b8f 100644 --- a/pkg/logql/evaluator_test.go +++ b/pkg/logql/evaluator_test.go @@ -20,6 +20,7 @@ func TestDefaultEvaluator_DivideByZero(t *testing.T) { }, false, false, + false, ) require.NoError(t, err) @@ -33,6 +34,7 @@ func TestDefaultEvaluator_DivideByZero(t *testing.T) { }, false, false, + false, ) require.NoError(t, err) require.Equal(t, true, math.IsNaN(binOp.F)) @@ -220,14 +222,14 @@ func TestEvaluator_mergeBinOpComparisons(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { // comparing a binop should yield the unfiltered (non-nil variant) regardless // of whether this is a vector-vector comparison or not. - op, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, false) + op, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, false, false) require.NoError(t, err) require.Equal(t, tc.expected, op) - op2, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, true) + op2, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, false, true) require.NoError(t, err) require.Equal(t, tc.expected, op2) - op3, err := syntax.MergeBinOp(tc.op, tc.lhs, nil, false, true) + op3, err := syntax.MergeBinOp(tc.op, tc.lhs, nil, false, false, true) require.NoError(t, err) require.Nil(t, op3) @@ -235,16 +237,16 @@ func TestEvaluator_mergeBinOpComparisons(t *testing.T) { if tc.expected.F == 0 { // ensure zeroed predicates are filtered out - op, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, true, false) + op, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, true, false) require.NoError(t, err) require.Nil(t, op) - op2, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, true, true) + op2, err := syntax.MergeBinOp(tc.op, tc.lhs, tc.rhs, false, true, true) require.NoError(t, err) require.Nil(t, op2) // for vector-vector comparisons, ensure that nil right hand sides // translate into nil results - op3, err := syntax.MergeBinOp(tc.op, tc.lhs, nil, true, true) + op3, err := syntax.MergeBinOp(tc.op, tc.lhs, nil, false, true, true) require.NoError(t, err) require.Nil(t, op3) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 2deda9ff612cc..82780225837f7 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -1631,6 +1631,7 @@ func reduceBinOp(op string, left, right float64) *LiteralExpr { &promql.Sample{F: right}, false, false, + false, ) if err != nil { return &LiteralExpr{err: err} @@ -1638,7 +1639,12 @@ func reduceBinOp(op string, left, right float64) *LiteralExpr { return &LiteralExpr{Val: merged.F} } -func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorComparison bool) (*promql.Sample, error) { +// MergeBinOp performs `op` on `left` and `right` arguments and return the `promql.Sample` value. +// In case of vector and scalar arguments, MergeBinOp assumes `left` is always vector. +// pass `swap=true` otherwise. +// This matters because, either it's (vector op scalar) or (scalar op vector), the return sample value is +// always sample value of vector argument. +func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVectorComparison bool) (*promql.Sample, error) { var merger func(left, right *promql.Sample) *promql.Sample switch op { @@ -1724,7 +1730,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F == right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1741,7 +1747,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F != right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1758,7 +1764,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F > right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1775,7 +1781,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F >= right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1792,7 +1798,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F < right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1809,7 +1815,7 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso val := 0. if left.F <= right.F { val = 1. - } else if filter { + } else if notReturnBool { return nil } res.F = val @@ -1825,10 +1831,14 @@ func MergeBinOp(op string, left, right *promql.Sample, filter, isVectorCompariso return res, nil } - if filter { - // if a filter-enabled vector-wise comparison has returned non-nil, + if notReturnBool { + if swap { + left, right = right, left + } + + // if a notReturnBool is enabled vector-wise comparison has returned non-nil, // ensure we return the left hand side's value (2) instead of the - // comparison operator's result (1: the truthy answer) + // comparison operator's result (1: the truthy answer. a.k.a bool) if res != nil { return left, nil } diff --git a/pkg/logql/syntax/ast_test.go b/pkg/logql/syntax/ast_test.go index 0ffbc3851e407..e1570e07e8c1f 100644 --- a/pkg/logql/syntax/ast_test.go +++ b/pkg/logql/syntax/ast_test.go @@ -177,10 +177,10 @@ func Test_SampleExpr_String(t *testing.T) { ) `, `((( - sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-1min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 60) - or on () - ((sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-15min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 15) / 60)) - or on () + sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-1min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 60) + or on () + ((sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-15min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 15) / 60)) + or on () ((sum by(typename,pool,commandname,colo) (sum_over_time({_namespace_="appspace", _schema_="appspace-1h", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 60) / 60))`, `{app="foo"} | logfmt code="response.code", IPAddress="host"`, } { @@ -670,6 +670,7 @@ func Test_MergeBinOpVectors_Filter(t *testing.T) { OpTypeGT, &promql.Sample{F: 2}, &promql.Sample{F: 0}, + false, true, true, ) From 6aaf69227ee65b70d71f08563db2a5a75db61e1a Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Mon, 23 Oct 2023 09:18:57 +0200 Subject: [PATCH 2/8] move tests from bug_test to engine_test.go Signed-off-by: Kaviraj --- pkg/logql/engine_test.go | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/pkg/logql/engine_test.go b/pkg/logql/engine_test.go index c5a1c7235492a..75853da0e3f64 100644 --- a/pkg/logql/engine_test.go +++ b/pkg/logql/engine_test.go @@ -675,6 +675,25 @@ func TestEngine_LogsInstantQuery(t *testing.T) { {T: 60 * 1000, F: 60, Metric: labels.FromStrings("app", "foo")}, }, }, + { + // should return same results as `count_over_time({app="foo"}[1m]) > 1`. + // https://grafana.com/docs/loki/latest/query/#comparison-operators + // Between a vector and a scalar, these operators are + // applied to the value of every data sample in the vector + `1 < count_over_time({app="foo"}[1m])`, + time.Unix(60, 0), + logproto.FORWARD, + 0, + [][]logproto.Series{ + {newSeries(testSize, identity, `{app="foo"}`)}, + }, + []SelectSampleParams{ + {&logproto.SampleQueryRequest{Start: time.Unix(0, 0), End: time.Unix(60, 0), Selector: `count_over_time({app="foo"}[1m])`}}, + }, + promql.Vector{ + {T: 60 * 1000, F: 60, Metric: labels.FromStrings("app", "foo")}, + }, + }, { `count_over_time({app="foo"}[1m]) > count_over_time({app="bar"}[1m])`, time.Unix(60, 0), @@ -2107,6 +2126,34 @@ func TestEngine_RangeQuery(t *testing.T) { }, }, }, + { + // should return same results as `bytes_over_time({app="foo"}[30s]) > 1`. + // https://grafana.com/docs/loki/latest/query/#comparison-operators + // Between a vector and a scalar, these operators are + // applied to the value of every data sample in the vector + `1 < bytes_over_time({app="foo"}[30s])`, time.Unix(60, 0), time.Unix(120, 0), 15 * time.Second, 0, logproto.FORWARD, 10, + [][]logproto.Series{ + {logproto.Series{ + Labels: `{app="foo"}`, + Samples: []logproto.Sample{ + {Timestamp: time.Unix(45, 0).UnixNano(), Hash: 1, Value: 5.}, // 5 bytes + {Timestamp: time.Unix(60, 0).UnixNano(), Hash: 2, Value: 0.}, + {Timestamp: time.Unix(75, 0).UnixNano(), Hash: 3, Value: 0.}, + {Timestamp: time.Unix(90, 0).UnixNano(), Hash: 4, Value: 0.}, + {Timestamp: time.Unix(105, 0).UnixNano(), Hash: 5, Value: 4.}, // 4 bytes + }, + }}, + }, + []SelectSampleParams{ + {&logproto.SampleQueryRequest{Start: time.Unix(30, 0), End: time.Unix(120, 0), Selector: `bytes_over_time({app="foo"}[30s])`}}, + }, + promql.Matrix{ + promql.Series{ + Metric: labels.FromStrings("app", "foo"), + Floats: []promql.FPoint{{T: 60 * 1000, F: 5.}, {T: 105 * 1000, F: 4.}, {T: 120 * 1000, F: 4.}}, + }, + }, + }, { `bytes_over_time({app="foo"}[30s]) > bool 1`, time.Unix(60, 0), time.Unix(120, 0), 15 * time.Second, 0, logproto.FORWARD, 10, [][]logproto.Series{ From bf9a0c9fcf07f3f3290e9f074015f96e87e23f89 Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Mon, 23 Oct 2023 09:35:43 +0200 Subject: [PATCH 3/8] add test for literalevaluator Signed-off-by: Kaviraj --- pkg/logql/evaluator_test.go | 89 +++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/pkg/logql/evaluator_test.go b/pkg/logql/evaluator_test.go index 9f0ddebbf0b8f..1bec3d9c67d68 100644 --- a/pkg/logql/evaluator_test.go +++ b/pkg/logql/evaluator_test.go @@ -3,8 +3,11 @@ package logql import ( "math" "testing" + "time" + "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/promql" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/loki/pkg/logql/syntax" @@ -282,6 +285,52 @@ func TestEmptyNestedEvaluator(t *testing.T) { } +func TestLiteralStepEvaluator(t *testing.T) { + cases := []struct { + name string + expr *LiteralStepEvaluator + expected []float64 + }{ + { + name: "vector op scalar", + // e.g: sum(count_over_time({app="foo"}[1m])) > 20 + expr: &LiteralStepEvaluator{ + nextEv: newReturnVectorEvaluator([]float64{20, 21, 22, 23}), + val: 20, + inverted: true, // set to true, because literal expression is not on left. + op: ">", + }, + expected: []float64{21, 22, 23}, + }, + { + name: "scalar op vector", + // e.g: 20 < sum(count_over_time({app="foo"}[1m])) + expr: &LiteralStepEvaluator{ + nextEv: newReturnVectorEvaluator([]float64{20, 21, 22, 23}), + val: 20, + inverted: false, + op: "<", + }, + expected: []float64{21, 22, 23}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ok, _, got := tc.expr.Next() + require.True(t, ok) + vecs := got.SampleVector() + gotSamples := make([]float64, 0, len(vecs)) + + for _, v := range vecs { + gotSamples = append(gotSamples, v.F) + } + + assert.Equal(t, tc.expected, gotSamples) + }) + } +} + type emptyEvaluator struct{} func (*emptyEvaluator) Next() (ok bool, ts int64, r StepResult) { @@ -297,3 +346,43 @@ func (*emptyEvaluator) Error() error { } func (*emptyEvaluator) Explain(Node) {} + +// returnVectorEvaluator returns elements of vector +// passed in, everytime it's `Next()` is called. Used for testing. +type returnVectorEvaluator struct { + vec promql.Vector +} + +func (e *returnVectorEvaluator) Next() (ok bool, ts int64, r StepResult) { + return true, 0, SampleVector(e.vec) +} + +func (*returnVectorEvaluator) Close() error { + return nil +} + +func (*returnVectorEvaluator) Error() error { + return nil +} + +func (*returnVectorEvaluator) Explain(Node) { + +} + +func newReturnVectorEvaluator(vec []float64) *returnVectorEvaluator { + testTime := time.Now().Unix() + + pvec := make([]promql.Sample, 0, len(vec)) + + for _, v := range vec { + pvec = append(pvec, promql.Sample{ + T: testTime, + F: v, + Metric: labels.FromStrings("foo", "bar"), + }) + } + + return &returnVectorEvaluator{ + vec: pvec, + } +} From bd0f82b77b8ac027023d4c5767e960afe469201f Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Mon, 23 Oct 2023 09:36:03 +0200 Subject: [PATCH 4/8] remove `bug_test.go` as it is moved to engine_test.go Signed-off-by: Kaviraj --- pkg/logql/bug_test.go | 94 ------------------------------------------- 1 file changed, 94 deletions(-) delete mode 100644 pkg/logql/bug_test.go diff --git a/pkg/logql/bug_test.go b/pkg/logql/bug_test.go deleted file mode 100644 index 7c6ba3075293d..0000000000000 --- a/pkg/logql/bug_test.go +++ /dev/null @@ -1,94 +0,0 @@ -package logql - -import ( - "context" - "fmt" - "math" - "testing" - "time" - - "github.com/grafana/dskit/user" - "github.com/grafana/loki/pkg/logproto" - "github.com/stretchr/testify/require" - - "github.com/go-kit/log" -) - -func TestScalarVectorBinOp(t *testing.T) { - NoLimits = &fakeLimits{maxSeries: math.MaxInt32} - - qs1 := `sum(count_over_time({app="foo"}[1m])) > 3` - qs2 := `3 < sum(count_over_time({app="foo"}[1m]))` - - data := [][]logproto.Series{ - {newSeries(4, constant(70), `{app="foo",pool="foo"}`)}, - } - - fmt.Printf("%+v\n", data) - - params := []SelectSampleParams{ - {&logproto.SampleQueryRequest{Start: time.Unix(10, 0), End: time.Unix(70, 0), Selector: `sum(count_over_time({app="foo"}[1m]))`}}, - } - - querier := newQuerierRecorder(t, data, params) - - engine := NewEngine(EngineOpts{}, querier, NoLimits, log.NewNopLogger()) - start := time.Unix(70, 0) - end := time.Unix(70, 0) - direction := logproto.FORWARD - limit := uint32(0) - - q1 := engine.Query(LiteralParams{qs: qs1, start: start, end: end, direction: direction, limit: limit}) - res1, err := q1.Exec(user.InjectOrgID(context.Background(), "fake")) - if err != nil { - t.Fatal(err) - } - - q2 := engine.Query(LiteralParams{qs: qs2, start: start, end: end, direction: direction, limit: limit}) - res2, err := q2.Exec(user.InjectOrgID(context.Background(), "fake")) - if err != nil { - t.Fatal(err) - } - - require.Equal(t, res1.Data, res2.Data) - - // fmt.Println("Return Data type:", res.Data.Type()) - // fmt.Printf("%+v\n", res.Data) -} - -// func TestBugNotWorking(t *testing.T) { -// NoLimits = &fakeLimits{maxSeries: math.MaxInt32} - -// data := [][]logproto.Series{ -// {newSeries(4, constant(70), `{app="foo",pool="foo"}`)}, -// } - -// params := []SelectSampleParams{ -// {&logproto.SampleQueryRequest{Start: time.Unix(10, 0), End: time.Unix(70, 0), Selector: `sum(count_over_time({app="foo"}[1m]))`}}, -// } - -// querier := newQuerierRecorder(t, data, params) - -// engine := NewEngine(EngineOpts{}, querier, NoLimits, log.NewNopLogger()) -// start := time.Unix(70, 0) -// end := time.Unix(70, 0) -// direction := logproto.FORWARD -// limit := uint32(0) - -// q := engine.Query(LiteralParams{qs: qs, start: start, end: end, direction: direction, limit: limit}) -// res, err := q.Exec(user.InjectOrgID(context.Background(), "fake")) -// if err != nil { -// t.Fatal(err) -// } - -// fmt.Println("Return Data type:", res.Data.Type()) -// fmt.Printf("%+v\n", res.Data) -// } - -// func TestUnderstanding(t *testing.T) { -// data := [][]logproto.Series{ -// {newSeries(2, identity, `{app="foo"}`)}, -// } - -// fmt.Printf("%+v\n", data) -// } From 1e79dacc0206934e60751f55d3a6f79253b81042 Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Mon, 23 Oct 2023 09:44:41 +0200 Subject: [PATCH 5/8] Add doc reference Signed-off-by: Kaviraj --- pkg/logql/syntax/ast.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 82780225837f7..92ade31183fb6 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -1642,8 +1642,9 @@ func reduceBinOp(op string, left, right float64) *LiteralExpr { // MergeBinOp performs `op` on `left` and `right` arguments and return the `promql.Sample` value. // In case of vector and scalar arguments, MergeBinOp assumes `left` is always vector. // pass `swap=true` otherwise. -// This matters because, either it's (vector op scalar) or (scalar op vector), the return sample value is -// always sample value of vector argument. +// This matters because, either it's (vector op scalar) or (scalar op vector), the return sample value should +// always be sample value of vector argument. +// https://github.com/grafana/loki/issues/10741 func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVectorComparison bool) (*promql.Sample, error) { var merger func(left, right *promql.Sample) *promql.Sample From 675b8340ddfe25799e1dba782e7dcd2dd2933ef9 Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Mon, 23 Oct 2023 10:04:25 +0200 Subject: [PATCH 6/8] Fix linter error Signed-off-by: Kaviraj --- pkg/logql/syntax/ast.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 92ade31183fb6..83ece7df20322 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -1833,15 +1833,16 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe } if notReturnBool { - if swap { - left, right = right, left - } - // if a notReturnBool is enabled vector-wise comparison has returned non-nil, - // ensure we return the left hand side's value (2) instead of the + // ensure we return the vector hand side's sample value, instead of the // comparison operator's result (1: the truthy answer. a.k.a bool) + + retSample := left + if swap { + retSample = right + } if res != nil { - return left, nil + return retSample, nil } } return res, nil From bf30a6e9d253111742e3509a93fafbcfb3fa62ad Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Fri, 27 Oct 2023 14:09:46 +0200 Subject: [PATCH 7/8] PR remarks 1. revert `notReturnBool` -> `filter` 2. swap independent of `filter` flag Signed-off-by: Kaviraj --- pkg/logql/syntax/ast.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 83ece7df20322..3387eec637a9f 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -1645,7 +1645,7 @@ func reduceBinOp(op string, left, right float64) *LiteralExpr { // This matters because, either it's (vector op scalar) or (scalar op vector), the return sample value should // always be sample value of vector argument. // https://github.com/grafana/loki/issues/10741 -func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVectorComparison bool) (*promql.Sample, error) { +func MergeBinOp(op string, left, right *promql.Sample, swap, filter, isVectorComparison bool) (*promql.Sample, error) { var merger func(left, right *promql.Sample) *promql.Sample switch op { @@ -1731,7 +1731,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F == right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1748,7 +1748,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F != right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1765,7 +1765,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F > right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1782,7 +1782,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F >= right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1799,7 +1799,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F < right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1816,7 +1816,7 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe val := 0. if left.F <= right.F { val = 1. - } else if notReturnBool { + } else if filter { return nil } res.F = val @@ -1832,15 +1832,15 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, notReturnBool, isVe return res, nil } - if notReturnBool { - // if a notReturnBool is enabled vector-wise comparison has returned non-nil, + retSample := left + if swap { + retSample = right + } + + if filter { + // if a filter is enabled vector-wise comparison has returned non-nil, // ensure we return the vector hand side's sample value, instead of the // comparison operator's result (1: the truthy answer. a.k.a bool) - - retSample := left - if swap { - retSample = right - } if res != nil { return retSample, nil } From b95b37bbf08936ae242d5bd10991a30116283a57 Mon Sep 17 00:00:00 2001 From: Kaviraj Date: Fri, 27 Oct 2023 14:41:35 +0200 Subject: [PATCH 8/8] Add extra test to lock `bool` behaviour with vector and scalar Signed-off-by: Kaviraj --- pkg/logql/engine_test.go | 34 ++++++++++++++++++++++++++++++++++ pkg/logql/syntax/ast.go | 11 ++++++----- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/pkg/logql/engine_test.go b/pkg/logql/engine_test.go index 64034f3e21320..ef7d5e0538e3d 100644 --- a/pkg/logql/engine_test.go +++ b/pkg/logql/engine_test.go @@ -2184,6 +2184,40 @@ func TestEngine_RangeQuery(t *testing.T) { }, }, }, + { + // should return same results as `bytes_over_time({app="foo"}[30s]) > bool 1`. + // https://grafana.com/docs/loki/latest/query/#comparison-operators + // Between a vector and a scalar, these operators are + // applied to the value of every data sample in the vector + `1 < bool bytes_over_time({app="foo"}[30s])`, time.Unix(60, 0), time.Unix(120, 0), 15 * time.Second, 0, logproto.FORWARD, 10, + [][]logproto.Series{ + {logproto.Series{ + Labels: `{app="foo"}`, + Samples: []logproto.Sample{ + {Timestamp: time.Unix(45, 0).UnixNano(), Hash: 1, Value: 5.}, // 5 bytes + {Timestamp: time.Unix(60, 0).UnixNano(), Hash: 2, Value: 0.}, + {Timestamp: time.Unix(75, 0).UnixNano(), Hash: 3, Value: 0.}, + {Timestamp: time.Unix(90, 0).UnixNano(), Hash: 4, Value: 0.}, + {Timestamp: time.Unix(105, 0).UnixNano(), Hash: 5, Value: 4.}, // 4 bytes + }, + }}, + }, + []SelectSampleParams{ + {&logproto.SampleQueryRequest{Start: time.Unix(30, 0), End: time.Unix(120, 0), Selector: `bytes_over_time({app="foo"}[30s])`}}, + }, + promql.Matrix{ + promql.Series{ + Metric: labels.FromStrings("app", "foo"), + Floats: []promql.FPoint{ + {T: 60000, F: 1}, + {T: 75000, F: 0}, + {T: 90000, F: 0}, + {T: 105000, F: 1}, + {T: 120000, F: 1}, + }, + }, + }, + }, { // tests combining two streams + unwrap `sum(rate({job="foo"} | logfmt | bar > 0 | unwrap bazz [30s]))`, time.Unix(60, 0), time.Unix(120, 0), 30 * time.Second, 0, logproto.FORWARD, 10, diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 3387eec637a9f..aa4aa7fa80617 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -1832,15 +1832,16 @@ func MergeBinOp(op string, left, right *promql.Sample, swap, filter, isVectorCom return res, nil } - retSample := left - if swap { - retSample = right - } - if filter { // if a filter is enabled vector-wise comparison has returned non-nil, // ensure we return the vector hand side's sample value, instead of the // comparison operator's result (1: the truthy answer. a.k.a bool) + + retSample := left + if swap { + retSample = right + } + if res != nil { return retSample, nil }