Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify Scalar Calculation #124

Closed
wants to merge 2 commits into from
Closed

Simplify Scalar Calculation #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d637ca2
enabling scalar tests
alanprot Nov 10, 2022
b7b0c25
Removing uneeded code from scalar calculation
alanprot Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
alanprot marked this conversation as resolved.
Show resolved Hide resolved
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{
Expand Down
72 changes: 38 additions & 34 deletions engine/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package engine_test

import (
"context"
"encoding/json"
"fmt"
"math"
"runtime"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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)
})
}
})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}
})
Expand Down Expand Up @@ -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)
}
8 changes: 2 additions & 6 deletions execution/function/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand Down