From 2cf2bf5b8f0ea473f5626fb396f6346715a5ead8 Mon Sep 17 00:00:00 2001 From: Julien Pivotto <291750+roidelapluie@users.noreply.github.com> Date: Thu, 3 Apr 2025 12:48:46 +0200 Subject: [PATCH] Address feedback Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com> --- promql/durations.go | 9 +- promql/durations_test.go | 257 +++++++++++++++------------------------ 2 files changed, 99 insertions(+), 167 deletions(-) diff --git a/promql/durations.go b/promql/durations.go index c047ac0bfc..8431fa5bd4 100644 --- a/promql/durations.go +++ b/promql/durations.go @@ -14,7 +14,6 @@ package promql import ( - "errors" "fmt" "math" "time" @@ -76,10 +75,10 @@ func calculateDuration(expr parser.Expr, allowedNegative bool) (time.Duration, e return 0, err } if duration <= 0 && !allowedNegative { - return 0, errors.New("duration must be greater than 0") + return 0, fmt.Errorf("%d:%d: duration must be greater than 0", expr.PositionRange().Start, expr.PositionRange().End) } if duration > 1<<63-1 || duration < -1<<63 { - return 0, errors.New("duration is out of range") + return 0, fmt.Errorf("%d:%d: duration is out of range", expr.PositionRange().Start, expr.PositionRange().End) } return time.Duration(duration*1000) * time.Millisecond, nil } @@ -118,12 +117,12 @@ func evaluateDurationExpr(expr parser.Expr) (float64, error) { return lhs * rhs, nil case parser.DIV: if rhs == 0 { - return 0, errors.New("division by zero") + return 0, fmt.Errorf("%d:%d: division by zero", expr.PositionRange().Start, expr.PositionRange().End) } return lhs / rhs, nil case parser.MOD: if rhs == 0 { - return 0, errors.New("modulo by zero") + return 0, fmt.Errorf("%d:%d: modulo by zero", expr.PositionRange().Start, expr.PositionRange().End) } return math.Mod(lhs, rhs), nil case parser.POW: diff --git a/promql/durations_test.go b/promql/durations_test.go index d1ac4b77d7..e86a03db1a 100644 --- a/promql/durations_test.go +++ b/promql/durations_test.go @@ -99,206 +99,139 @@ func TestDurationVisitor(t *testing.T) { } func TestCalculateDuration(t *testing.T) { - // Enable experimental duration expression parsing. - parser.ExperimentalDurationExpr = true - t.Cleanup(func() { - parser.ExperimentalDurationExpr = false - }) tests := []struct { - name string - expr string - expected time.Duration - expectError bool + name string + expr parser.Expr + expected time.Duration + errorMessage string + requirePositive bool }{ { - name: "number literal", - expr: "5", - expected: 5 * time.Second, - }, - { - name: "time unit literal", - expr: "1h", - expected: time.Hour, - }, - { - name: "addition with numbers", - expr: "5 + 10", + name: "addition", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.NumberLiteral{Val: 10}, + Op: parser.ADD, + }, expected: 15 * time.Second, }, { - name: "addition with time units", - expr: "1h + 30m", - expected: 90 * time.Minute, - }, - { - name: "subtraction with numbers", - expr: "15 - 5", + name: "subtraction", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 15}, + RHS: &parser.NumberLiteral{Val: 5}, + Op: parser.SUB, + }, expected: 10 * time.Second, }, { - name: "subtraction with time units", - expr: "2h - 30m", - expected: 90 * time.Minute, + name: "subtraction with negative", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.NumberLiteral{Val: 10}, + Op: parser.SUB, + }, + errorMessage: "duration must be greater than 0", }, { - name: "multiplication with numbers", - expr: "5 * 3", + name: "multiplication", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.NumberLiteral{Val: 3}, + Op: parser.MUL, + }, expected: 15 * time.Second, }, { - name: "multiplication with time unit and number", - expr: "2h * 1.5", - expected: 3 * time.Hour, - }, - { - name: "division with numbers", - expr: "15 / 3", + name: "division", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 15}, + RHS: &parser.NumberLiteral{Val: 3}, + Op: parser.DIV, + }, expected: 5 * time.Second, }, { - name: "division with time unit and number", - expr: "1h / 2", - expected: 30 * time.Minute, - }, - { - name: "modulo with numbers", - expr: "17 % 5", + name: "modulo with numbers", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 17}, + RHS: &parser.NumberLiteral{Val: 5}, + Op: parser.MOD, + }, expected: 2 * time.Second, }, { - name: "modulo with time unit and number", - expr: "70m % 60m", - expected: 10 * time.Minute, - }, - { - name: "power with numbers", - expr: "2 ^ 3", + name: "power", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 2}, + RHS: &parser.NumberLiteral{Val: 3}, + Op: parser.POW, + }, expected: 8 * time.Second, }, { - name: "complex expression with numbers", - expr: "2 * (3 + 4) - 1", + name: "complex expression", + expr: &parser.DurationExpr{ + LHS: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 2}, + RHS: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 3}, + RHS: &parser.NumberLiteral{Val: 4}, + Op: parser.ADD, + }, + Op: parser.MUL, + }, + RHS: &parser.NumberLiteral{Val: 1}, + Op: parser.SUB, + }, expected: 13 * time.Second, }, { - name: "complex expression with time units", - expr: "2 * (1h + 30m) - 15m", - expected: 165 * time.Minute, + name: "unary negative", + expr: &parser.DurationExpr{ + RHS: &parser.NumberLiteral{Val: 5}, + Op: parser.SUB, + }, + expected: -5 * time.Second, + requirePositive: true, }, { - name: "unary negative with number", - expr: "-5", - expected: -5 * time.Second, + name: "division by zero", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.NumberLiteral{Val: 5}, + Op: parser.SUB, + }, + Op: parser.DIV, + }, + errorMessage: "division by zero", }, { - name: "unary negative with time unit", - expr: "-1h", - expected: -time.Hour, - }, - { - name: "division by zero with numbers", - expr: "5 / 0", - expectError: true, - }, - { - name: "division by zero with time unit", - expr: "1h / 0", - expectError: true, - }, - { - name: "modulo by zero with numbers", - expr: "5 % 0", - expectError: true, - }, - { - name: "modulo by zero with time unit", - expr: "1h % 0", - expectError: true, + name: "modulo by zero", + expr: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.DurationExpr{ + LHS: &parser.NumberLiteral{Val: 5}, + RHS: &parser.NumberLiteral{Val: 5}, + Op: parser.SUB, + }, + Op: parser.MOD, + }, + errorMessage: "modulo by zero", }, } for _, tt := range tests { - t.Run(tt.name+" offset", func(t *testing.T) { - expr, err := parser.ParseExpr("foo offset (" + tt.expr + ")") - if tt.expectError { + t.Run(tt.name, func(t *testing.T) { + result, err := calculateDuration(tt.expr, tt.requirePositive) + if tt.errorMessage != "" { require.Error(t, err) + require.Contains(t, err.Error(), tt.errorMessage) return } require.NoError(t, err) - - // Extract the duration expression from the vector selector - vectorSelector, ok := expr.(*parser.VectorSelector) - require.True(t, ok, "Expected vector selector, got %T", expr) - - result := vectorSelector.OriginalOffset - if vectorSelector.OriginalOffsetExpr != nil { - result, err = calculateDuration(vectorSelector.OriginalOffsetExpr, false) - require.NoError(t, err) - } - require.Equal(t, tt.expected, result) - }) - - t.Run(tt.name+" subquery with fixed step", func(t *testing.T) { - expr, err := parser.ParseExpr("foo[5m:(" + tt.expr + ")]") - if tt.expectError || tt.expected < 0 { - require.Error(t, err) - return - } - require.NoError(t, err) - - // Extract the duration expression from the subquery - subquery, ok := expr.(*parser.SubqueryExpr) - require.True(t, ok, "Expected subquery, got %T", expr) - - require.Equal(t, 5*time.Minute, subquery.Range) - - result := subquery.Step - if subquery.StepExpr != nil { - result, err = calculateDuration(subquery.StepExpr, false) - require.NoError(t, err) - } - require.Equal(t, tt.expected, result) - }) - - t.Run(tt.name+" subquery with fixed range", func(t *testing.T) { - expr, err := parser.ParseExpr("foo[(" + tt.expr + "):5m]") - if tt.expectError || tt.expected < 0 { - require.Error(t, err) - return - } - require.NoError(t, err) - - // Extract the duration expression from the subquery - subquery, ok := expr.(*parser.SubqueryExpr) - require.True(t, ok, "Expected subquery, got %T", expr) - - require.Equal(t, 5*time.Minute, subquery.Step) - - result := subquery.Range - if subquery.RangeExpr != nil { - result, err = calculateDuration(subquery.RangeExpr, false) - require.NoError(t, err) - } - require.Equal(t, tt.expected, result) - }) - - t.Run(tt.name+" matrix selector", func(t *testing.T) { - expr, err := parser.ParseExpr("foo[(" + tt.expr + ")]") - if tt.expectError || tt.expected < 0 { - require.Error(t, err) - return - } - require.NoError(t, err) - - // Extract the duration expression from the matrix selector - matrixSelector, ok := expr.(*parser.MatrixSelector) - require.True(t, ok, "Expected matrix selector, got %T", expr) - - result := matrixSelector.Range - if matrixSelector.RangeExpr != nil { - result, err = calculateDuration(matrixSelector.RangeExpr, false) - require.NoError(t, err) - } require.Equal(t, tt.expected, result) }) }