From d637ca2fee517ab25948562ff0be47ac4a423920 Mon Sep 17 00:00:00 2001 From: Alan Protasio Date: Thu, 10 Nov 2022 13:50:49 -0800 Subject: [PATCH 1/2] enabling scalar tests --- engine/engine_test.go | 72 +++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/engine/engine_test.go b/engine/engine_test.go index ca4a2390..e39d3d38 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -5,6 +5,7 @@ package engine_test import ( "context" + "encoding/json" "fmt" "math" "runtime" @@ -820,15 +821,13 @@ func TestQueriesAgainstOldEngine(t *testing.T) { + on() group_left() sum(http_requests_total{ns="nginx"})`, }, - // Result is correct but this likely fails due to https://github.com/golang/go/issues/12025. - // TODO(saswatamcode): Test NaN cases separately. https://github.com/thanos-community/promql-engine/issues/88 - // { - // name: "scalar func with NaN", - // load: `load 30s - // http_requests_total{pod="nginx-1"} 1+1x15 - // http_requests_total{pod="nginx-2"} 1+2x18`, - // query: `scalar(http_requests_total)`, - // }, + { + name: "scalar func with NaN", + load: `load 30s + http_requests_total{pod="nginx-1"} 1+1x15 + http_requests_total{pod="nginx-2"} 1+2x18`, + query: `scalar(http_requests_total)`, + }, { name: "scalar func with aggr", load: `load 30s @@ -933,20 +932,19 @@ func TestQueriesAgainstOldEngine(t *testing.T) { http_requests_total{pod="nginx-2", le="+Inf"} 4+1x10`, query: `histogram_quantile(0.9, sum by (pod, le) (http_requests_total))`, }, - // TODO(fpetkovski): Uncomment once support for testing NaNs is added. - //{ - // name: "histogram quantile with scalar operator", - // load: `load 30s - // quantile{pod="nginx-1", le="1"} 1+1x2 - // http_requests_total{pod="nginx-1", le="1"} 1+3x10 - // http_requests_total{pod="nginx-2", le="1"} 2+3x10 - // http_requests_total{pod="nginx-1", le="2"} 1+2x10 - // http_requests_total{pod="nginx-2", le="2"} 2+2x10 - // http_requests_total{pod="nginx-2", le="5"} 3+2x10 - // http_requests_total{pod="nginx-1", le="+Inf"} 1+1x10 - // http_requests_total{pod="nginx-2", le="+Inf"} 4+1x10`, - // query: `histogram_quantile(scalar(max(quantile)), http_requests_total)`, - //}, + { + name: "histogram quantile with scalar operator", + load: `load 30s + quantile{pod="nginx-1", le="1"} 1+1x2 + http_requests_total{pod="nginx-1", le="1"} 1+3x10 + http_requests_total{pod="nginx-2", le="1"} 2+3x10 + http_requests_total{pod="nginx-1", le="2"} 1+2x10 + http_requests_total{pod="nginx-2", le="2"} 2+2x10 + http_requests_total{pod="nginx-2", le="5"} 3+2x10 + http_requests_total{pod="nginx-1", le="+Inf"} 1+1x10 + http_requests_total{pod="nginx-2", le="+Inf"} 4+1x10`, + query: `histogram_quantile(scalar(max(quantile)), http_requests_total)`, + }, } disableOptimizerOpts := []bool{true, false} @@ -990,7 +988,7 @@ func TestQueriesAgainstOldEngine(t *testing.T) { oldResult := q2.Exec(context.Background()) testutil.Ok(t, oldResult.Err) - testutil.Equals(t, oldResult, newResult) + jsonEqual(t, oldResult, newResult) }) } }) @@ -1526,15 +1524,13 @@ func TestInstantQuery(t *testing.T) { http_requests_total{pod="nginx-2"} 1+2x18`, query: "sum_over_time(http_requests_total[5m] @ 180 offset 2m)", }, - // Result is correct but this likely fails due to https://github.com/golang/go/issues/12025. - // TODO(saswatamcode): Test NaN cases separately. https://github.com/thanos-community/promql-engine/issues/88 - // { - // name: "scalar func with NaN", - // load: `load 30s - // http_requests_total{pod="nginx-1"} 1+1x15 - // http_requests_total{pod="nginx-2"} 1+2x18`, - // query: `scalar(http_requests_total)`, - // }, + { + name: "scalar func with NaN", + load: `load 30s + http_requests_total{pod="nginx-1"} 1+1x15 + http_requests_total{pod="nginx-2"} 1+2x18`, + query: `scalar(http_requests_total)`, + }, { name: "scalar func with aggr", load: `load 30s @@ -1644,7 +1640,7 @@ func TestInstantQuery(t *testing.T) { oldResult := q2.Exec(context.Background()) testutil.Ok(t, oldResult.Err) - testutil.Equals(t, oldResult, newResult) + jsonEqual(t, oldResult, newResult) }) } }) @@ -2132,3 +2128,11 @@ func TestEngineRecoversFromPanic(t *testing.T) { }) } + +func jsonEqual(t *testing.T, expected interface{}, actual interface{}) { + e, err := json.Marshal(expected) + testutil.Ok(t, err) + a, err := json.Marshal(actual) + testutil.Ok(t, err) + testutil.Equals(t, e, a) +} From b7b0c25b8e8eafdc0e521b45e74440723207360b Mon Sep 17 00:00:00 2001 From: Alan Protasio Date: Thu, 10 Nov 2022 14:43:22 -0800 Subject: [PATCH 2/2] Removing uneeded code from scalar calculation --- engine/engine.go | 23 ----------------------- execution/function/operator.go | 8 ++------ 2 files changed, 2 insertions(+), 29 deletions(-) diff --git a/engine/engine.go b/engine/engine.go index 952b0601..7bb45aef 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -216,29 +216,6 @@ loop: break loop } - // Case where Series call might return nil, but samples are present. - // For example scalar(http_request_total) where http_request_total has multiple values. - if len(series) == 0 && len(r) != 0 { - numSeries := 0 - for i := range r { - numSeries += len(r[i].Samples) - } - - series = make([]promql.Series, numSeries) - - for _, vector := range r { - for i := range vector.Samples { - series[i].Points = append(series[i].Points, promql.Point{ - T: vector.T, - V: vector.Samples[i], - }) - } - q.Query.exec.GetPool().PutStepVector(vector) - } - q.Query.exec.GetPool().PutVectors(r) - continue - } - for _, vector := range r { for i, s := range vector.SampleIDs { series[s].Points = append(series[s].Points, promql.Point{ diff --git a/execution/function/operator.go b/execution/function/operator.go index 82e402ce..b7f3aa5a 100644 --- a/execution/function/operator.go +++ b/execution/function/operator.go @@ -134,6 +134,7 @@ func (o *functionOperator) Next(ctx context.Context) ([]model.StepVector, error) vectors[batchIndex].Samples = vector.Samples[:1] vectors[batchIndex].SampleIDs = vector.SampleIDs[:1] + vector.SampleIDs[0] = 0 vector.Samples[0] = math.NaN() continue } @@ -158,16 +159,11 @@ func (o *functionOperator) Next(ctx context.Context) ([]model.StepVector, error) func (o *functionOperator) loadSeries(ctx context.Context) error { var err error o.once.Do(func() { - if o.funcExpr.Func.Name == "vector" { + if o.funcExpr.Func.Name == "vector" || o.funcExpr.Func.Name == "scalar" { o.series = []labels.Labels{labels.New()} return } - if o.funcExpr.Func.Name == "scalar" { - o.series = []labels.Labels{} - return - } - series, loadErr := o.nextOps[o.vectorIndex].Series(ctx) if loadErr != nil { err = loadErr