feat(promql): offset from time expressions by krajorama · Pull Request #15899 · prometheus/prometheus

krajorama · 2025-01-29T10:34:48Z

Please squash in when merging.

Related to #12318 .

In this iteration we implement the following as a first step:

Allow using a PromQL expression for setting the offset, provided that:

the expression evaluates to a scalar
the expression does not contain vector selector (no TSDB access)
the expression does not call the info() function (no TSDB access)
the time() function is only called on the top level of the expression, not inside a subquery

We'll call such expression a time expression.

During parsing we no longer set OriginalOffset field in sub-queries and vector selectors, instead we set a new
OriginalOffsetExpr field to the time expression, even if the expression is a literal number.

Before evaluating the overall PromQL expression, the engine shall evaluate the time expressions and set the OriginalOffset field in vector selectors and subqueries to the calculated value. This makes the change fairly non intrusive. Later we can change the code to start using the expression more directly. Note: this prohibits using time() function inside a sub-query as the evaluation time is not constant inside a sub-query.

Literals: https://prometheus.io/docs/prometheus/latest/querying/basics/#float-literals-and-time-durations

Related to #12318 . Allow using a PromQL expression for setting the offset, provided that: the expression evaluates to a scalar the expression does not contain vector selector (no TSDB access) the expression does not call the info() function (no TSDB access) the time() function is only called on the top level of the expression, not inside a subquery We'll call such expression a time expression. During parsing we no longer set OriginalOffset field in sub-queries and vector selectors, instead we set a new OriginalOffsetExpr field to the time expression, even if the expression is a literal number. Before evaluating the overall PromQL expression, the engine shall evaluate the time expressions and set the OriginalOffset field in vector selectors and subqueries to the calculated value. This makes the change fairly non intrusive. Later we can change the code to start using the expression more directly. Note: this prohibits using time() function inside a sub-query as the evaluation time is not constant inside a sub-query. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

beorn7

Very cool that this works, and fascinating to read. I just have no idea if this is a sane approach. We should definitely loop in some experts to tell us if so. Maybe @juliusv or @aknuds1 for starters?

@juliusv can probably also tell us if there will be problems mirroring this in the frontend code (for syntax highlighting, auto completion, etc.). Which we need to do before merging this. And documentation, of course…

promql/promqltest/testdata/duration_arithmetic.test

beorn7 · 2025-02-06T17:58:20Z

promql/parser/printer.go

+		}
+		return fmt.Sprintf("%s%s", sign, model.Duration(time.Duration(v)*time.Second))
+	}
+	return e.String()


I guess we could have a DurationString method that works like the original String method just that it does what you have implemented above whenever it hits a NumberLiteral leave. (Not meant as an action item for this PR, just thinking aloud.)

I'll see the need when moving on to range:step and timestamps.

promql/parser/generated_parser.y

promql/parser/functions.go

beorn7 · 2025-02-06T18:08:04Z

promql/engine.go

 	return output, bufHelpers
 }
+
+// timeValueOf calculates the scalar time value of an expression in float64.


Currently, it's more a duration than a time value. I guess later we want to use the same for timestamps (with @), so maybe timeValue is a good word, but this should explain that it might stand for a duration in seconds or a timestamp in seconds.

beorn7 · 2025-02-06T18:36:37Z

Another thought about how to merge and deliver this: Is there a good way to put this behind a feature flag? This is one of those things that might easily kreep in because it is so handy, i.e. people might just use and like it in large numbers. And then it will be hard to pull out again in case we realize it was a dead alley, even if we marked it as experimental, because people might have used it without knowing it was experimental.

On the other hand, we did exactly that with the "numbers and timestamp/durations are the same" thing (declared it experimental without hiding it behind a feature flag). We were reasonably confident that we wouldn't change the feature dramatically. I guess the same is true here.

And most importantly, of course: We don't change any behavior of existing valid PromQL expressions. So maybe we could go without a feature flag.

krajorama · 2025-02-10T09:45:50Z

Another thought about how to merge and deliver this: Is there a good way to put this behind a feature flag? This is one of those things that might easily kreep in because it is so handy, i.e. people might just use and like it in large numbers. And then it will be hard to pull out again in case we realize it was a dead alley, even if we marked it as experimental, because people might have used it without knowing it was experimental.

On the other hand, we did exactly that with the "numbers and timestamp/durations are the same" thing (declared it experimental without hiding it behind a feature flag). We were reasonably confident that we wouldn't change the feature dramatically. I guess the same is true here.

And most importantly, of course: We don't change any behavior of existing valid PromQL expressions. So maybe we could go without a feature flag.

I think we can add a feature flag and already error out in the parser if it's not set and you still try to use time expression.

promql/engine.go

fionaliao · 2025-02-10T14:33:02Z

promql/engine.go

+		if nl, ok := expr.(*parser.NumberLiteral); ok {
+			return nl.Val, nil
+		}


I think we should check for a NumberLiteral before we check for the storage/eval time dependencies since that's likely going to be the most common expression type

I've updated this in 7cd13e2

fionaliao · 2025-02-10T14:35:59Z

promql/engine.go

+}
+
+func (tev timeExprVisitor) Visit(node parser.Node, path []parser.Node) (parser.Visitor, error) {
+	checkTimeExpr := func(expr parser.Expr) (float64, error) {


Why is checkTimeExpr is defined within Visit? It doesn't use any of the Visit variables/parameters.

Also I think evaluateTimeExpr would be a better name, since the result of the expression is returned

I've updated the name in 7cd13e2, but kept in Visit since that's the only function that uses this function

promql/engine.go

fionaliao · 2025-02-10T14:52:12Z

promql/parser/generated_parser.y

+                            if dur.Type() != ValueTypeScalar {
+                                yylex.(*parser).unexpected("offset", "time expression does not evaluate to a scalar")
+                            }


For function calls, type check of args is done in checkAST(). For consistency, I'd put the type checking of the time expression there too.

promql/engine.go

krajorama · 2025-02-11T16:01:28Z

I think we can add a feature flag and already error out in the parser if it's not set and you still try to use time expression.

But as we discussed IRL it's not required. Skipping feature flag for now.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

beorn7 · 2025-02-12T12:43:57Z

Note: Rebasing this should fix the CI tests.

beorn7 · 2025-02-12T12:45:44Z

@krajorama do you plan to update the "frontend" in this PR, too? (I can try and dig out similar changes for reference, if you don't know those already.)

It worries me if we merge something in PromQL itself that isn't supported in the UI (so you get red squiggly lines when trying the new feature, and autocomplete doesn't work).

beorn7

@fionaliao is already deeper in the parser woods than I would ever get, so I'll leave this part to her.

My one concern is about the time expression always being parenthesized. That's fine for the usual a + b, but it will feel weird and inconsistent if the parentheses aren't really needed from a syntax perspective.

And of course, we need the UI update (where the parser is mirrored), and it would give me peace of mind of @juliusv could double-check for any concerns.

docs/querying/basics.md

beorn7 · 2025-02-12T12:51:26Z

docs/querying/basics.md

+subqueries. For example, 5 minute offset may be calculated from adding 2 minutes
+and 180 seconds together:
+
+    http_requests_total offset (2m + 180)


The reader might wonder how this is of any use. ("I could just write 5m. 🤔")
Maybe we should add that this is useful if queries are generated via some templating system that isn't capable of doing this kind of math on its own.
And maybe we should add that we plan to add support for time expressions to @ and to range selectors and sub-queries. (Or maybe not. Just anticipating what users will ask when they read this.)

I think it's useful to clarify, updated 229652a

I also added your suggestion of mentioning we are planning to support it in more cases and also linking to the issue, maybe that way we might get some comments from people who are interested in the additional cases

beorn7 · 2025-02-12T12:57:06Z

promql/engine.go

+		if v[0].H != nil {
+			return 0, ErrTimeExprNotAFloat("vector with a histogram element")
+		}
+		return v[0].F, nil


Do we actually need to support this "one element vector" case? In the documentation, you say a time expression has to be a scalar. It's easy for the user to just say offset scalar(one_element_vector) if they need to. I would even say that allowing a one element vector with the implicit conversion here could be dangerous.

So maybe really just allow "proper" scalars, change the error to ErrTimeExprNotAScalar and then also get rid of all the elaborate error generation here because the user will easily see their mistake if they get the general ErrTimeExprNotAScalar error.

As far as I can tell, eval() mostly returns Matrix types, even if e.Type() == parser.ValueTypeScalar. For example binary exprs between scalars still use rangeEval which returns a Matrix with a single value.

prometheus/promql/engine.go

Lines 2061 to 2067 in 74d7d37

case *parser.BinaryExpr:

switch lt, rt := e.LHS.Type(), e.RHS.Type(); {

case lt == parser.ValueTypeScalar && rt == parser.ValueTypeScalar:

return ev.rangeEval(ctx, nil, func(v []parser.Value, _ [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, annotations.Annotations) {

val := scalarBinop(e.Op, v[0].(Vector)[0].F, v[1].(Vector)[0].F)

return append(enh.Out, Sample{F: val}), nil

}, e.LHS, e.RHS)

prometheus/promql/engine.go

Line 1308 in 74d7d37

func (ev *evaluator) rangeEval(ctx context.Context, prepSeries func(labels.Labels, *EvalSeriesHelper), funcCall func([]parser.Value, [][]EvalSeriesHelper, *EvalNodeHelper) (Vector, annotations.Annotations), exprs ...parser.Expr) (Matrix, annotations.Annotations) {

The check done in

prometheus/promql/parser/generated_parser.y

Lines 450 to 457 in 2f144dd

| expr OFFSET paren_expr

{

dur, _ := $3.(*ParenExpr)

if dur.Type() != ValueTypeScalar {

yylex.(*parser).unexpected("offset", "time expression does not evaluate to a scalar")

}

yylex.(*parser).addOffset($1, dur)

}

should ensure the value returned is a scalar (i.e. only a single value being returned for a vector or matrix) even if the actual value received is within a matrix. So we shouldn't reach the cases where there are multiple values returned.

There is also an additional check+panic in scalarValueOf

prometheus/promql/engine.go

Lines 4008 to 4011 in 0efd36e

// We can only use scalars as timestamp or duration.

if e.Type() != parser.ValueTypeScalar {

panic("parser should have discarded non scalar time expression")

}

beorn7 · 2025-02-12T13:10:13Z

promql/parser/generated_parser.y

+			    yylex.(*parser).addOffset($1, dur)
                            $$ = $1
                        }
+                | expr OFFSET paren_expr


Does this mean that the time expression has to be in parentheses? For example, if I need to use some PromQL functions to calculate my offset, I could shoehorn that like scalar(sqrt(vector(2))), but could I then write foo offset scalar(sqrt(vector(2))), or do I have to write foo offset (scalar(sqrt(vector(2)))) ? Looks like the latter.

Yes, the latter.

The problem is that currently when you write metric offset 5m + 1, metric offset 5m will be evaluated first and then + 1. Allowing offset expressions without parentheses would mean that 5m + 1 could be evaluated first and then the offset part, which will make old queries behave differently. The old queries would have to be rewritten in the format (metric offset 5m) + 1.

I can see this being confusing because a single number_duration_literal doesn't need brackets while everything else does. But I think it's better than the alternative.

I'm fully onboard with offset (5m +1 ). What I'm talking about are the other expression I mentioned, namely foo offset scalar(sqrt(vector(2))). What I want is that the latter works, and offset (5m +1 ) works, and metric offset 5m + 1 would still mean that we are having an offset of 5m, then evaluate metric at that time, and then we add 1 to the result. Or in other words, it should behave "as expected" :)

Hmm, maybe the problem is that offset isn't formally an operator, but a modifier. So it's not part of the operator precedence rules. But maybe we can still make it work? foo offset scalar(sqrt(vector(2))) doesn't even contain an operator. Is "function_call" a thing in the parser code? (Sorry for being so illiterate with parser generation.)

I've tried messing around in yacc to get this to work, haven't got there yet, but I noticed one thing around subqueries

some_metric offset 1m [1m:] currently means (some_metric offset 1m) [1m:]

While for binary operators, vector(0) + vector(0)[1m:] currently means vector(0) + (vector(0)[1m:]) (which errors)

If we treat offsets like binary operators then some_metric offset 1m [1m:] should mean some_metric offset (1m [1m:]) which is probably not what we want

We could have different rules for offsets vs other binary operators, but that starts to get confusing.

Are you suggesting we just allow functions and number literals without parentheses? And anything else (e.g. 1 + 1) has to go in parentheses to be evaluated as part of the offset time expression?

So like krajo/duration-arithmetic...fionaliao-function-bracketless

Yes, I think that will help. (Or maybe that's exactly what I want.)

So yeah, we cannot make offset formally an operator. It's a modifier. But we can probably say that the time expression has to be in parentheses if it is necessary to avoid ambiguities. (Later, when we will use time expressions as duration in range selectors, we won't need parentheses in those cases. So we cannot say that time expressions generally have to be in parentheses if they contain an operator or something.)

If we were introducing offsets for the first time and decided at that point that we wanted time expressions too, I would have preferred all expression types including just durations like 5m to be inside parentheses for less ambiguity.

With the current context, where numbers/durations are allowed without parentheses, and also with the @ modifier allowing start() and end() without parentheses, I'm okay with the rule being functions/numbers/durations don't need to be bracketed, everything else does.

I would wait for @krajorama's opinion as well though, in case he had additional reasons for the parentheses in all non-number and duration cases

I used parenthesis for everything other than number literals for simplicity and get to a working MVP.

To me the question is about documentation , UX: what's more complicated/confusing ?

offset time expression is a number or duration or an expression in parenthesis

offset time expression is a number, duration or function call or an expression inside parenthesis

offset time expression must be inside a parenthesis if otherwise the expression is ambiguous

I don't really want to force the user to figure out what's ambiguous and what's not, so I think we'll end up with something concrete (first two options or maybe a wider list?) anyway.

I can also agree to

With the current context, where numbers/durations are allowed without parentheses, and also with the @ modifier allowing start() and end() without parentheses, I'm okay with the rule being functions/numbers/durations don't need to be bracketed, everything else does.

I'll implement that.

I guess if we designed PromQL from scratch, we might have required parentheses around the "argument" of offset in all cases. We essentially didn't do that back then because we only ever allowed a duration literal as that "argument". Now it would be a pretty big change to suddenly require offset(5m) rather than offset 5m.

What I would argue for is to do something that meets what users are used to from other languages. And the rule there is usually that you are always allowed to use parentheses so that you don't have to care about operator precedence and other ambiguities, but that there is a set of rules in place to resolve those ambiguities if there are no parentheses present. And that's what I would like to implement. For offset, I think the quoted rule is equivalent in effect, i.e.:

With the current context, where numbers/durations are allowed without parentheses, and also with the @ modifier allowing start() and end() without parentheses, I'm okay with the rule being functions/numbers/durations don't need to be bracketed, everything else does.

It gets more interesting when we arrive at durations in a range selector. I guess we all agree that we want to allow foo[5m + 15s] and not enforce foo[(5m + 15s)] (although the latter should still be possible, of course). So the rule in the end cannot be that a time expression has parentheses unless it's just a single number or duration literal or a function call. The rule in the end will be that a time expression requires parentheses to avoid ambiguity (with the recommendation to use parentheses in doubt). Having different rules depending on the context is IMHO more confusing.

juliusv · 2025-02-12T14:53:22Z

And of course, we need the UI update (where the parser is mirrored), and it would give me peace of mind of @juliusv could double-check for any concerns.

While @Nexucis built & understands the autocompletion/highlighting/linting UI code way better than me, I suspect that it should be doable. Maybe some invalid queries (like accessing the TSDB) would only be detected during runtime evaluation and not by the linter though.

But since you asked me in general, my larger question (as an old curmudgeon) is of course going to be around whether it's is worth adding the conceptual complexity and special-casing ("it's like a normal expression but it's not") around a niche use case, or if this is actually a pretty common use case that I'm not aware of. I'm just wondering that because it's the first time I've seen that linked issue and someone requesting something like that. And in that issue it's still not clear to me whether they could solve their use case by just setting the overall query time parameter, or whether they require a range query with some regularly moving selectors and others that have a computed fixed timestamp.

beorn7 · 2025-02-13T13:09:26Z

I'm just wondering that because it's the first time I've seen that linked issue and someone requesting something like that. And in that issue it's still not clear to me whether they could solve their use case by just setting the overall query time parameter, or whether they require a range query with some regularly moving selectors and others that have a computed fixed timestamp.

To clarify, we just took the linked issue as the one umbrella issue, but arithmetic on durations and timestamps has been asked for repeatedly and for a long time. The more appropriate reference is probably my exploratory doc.

A fairly regular request I see in my Grafana bubble comes from Grafana users that use Grafana template variables and want to multiply them or add a number to them, but that's not supported in the Grafana templating. (It might be at some point in the future, but Prometheus might just have more velocity here, and there are also other reasons for Prometheus to have this.)

Another more visionary use case is utilization of start(), end(), and future pseudo functions like step() or even something for the scrape interval, which could be used to calculate reasonably range durations.

But since you asked me in general, my larger question (as an old curmudgeon) is of course going to be around whether it's is worth adding the conceptual complexity and special-casing ("it's like a normal expression but it's not") around a niche use case, or if this is actually a pretty common use case that I'm not aware of.

"Both", kind of… I think this is one of the more common remaining use cases (note that this PR only implements offset, but that's just the PoC. We want the same for @ and for range selectors and for sub-queries.

The other thing is that we also want to make "any" expression work (if that's feasible). So you could store a range interval in another series (like we do for alert thresholds) and things like that. Then the special case would disappear.

juliusv · 2025-02-13T20:28:41Z

@beorn7 Thanks for the additional context, yeah the Grafana multiplication use case makes the most sense to me.

We want the same for @ and for range selectors and for sub-queries.

In general, this would mean that we are splitting what is currently a single expression node into a parent and a child expression (with potentially more children). So in a PromLens-style tree view, even

my_metric{foo="bar"}[5m]

...would turn into something like:

my_metric{foo="bar"}[...]
    |
    +---- 5m

...unless we special-case simple duration literals again or choose to display them differently. Same for @ etc. Since this is still going to be a rarely used feature, we'd probably want to special-case simple literals so as not to affect normal users, but that's then a bit confusing that something looks like it's part of a single node in one case or becomes a full child node when it becomes more complex. I can't say I like either option much.

beorn7 · 2025-02-18T14:54:05Z

In general, this would mean that we are splitting what is currently a single expression node into a parent and a child expression (with potentially more children).

Yes, that's the idea at the core, so really unavoidable unless we don't do the feature at all.

As a UI improvement, it sounds OK to me to collapse the child structure into the current representation if a duration or timestamp is just a literal.

Co-authored-by: Björn Rabenstein <beorn@grafana.com> Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

juliusv · 2025-02-19T10:08:35Z

In general, this would mean that we are splitting what is currently a single expression node into a parent and a child expression (with potentially more children).

Yes, that's the idea at the core, so really unavoidable unless we don't do the feature at all.

As a UI improvement, it sounds OK to me to collapse the child structure into the current representation if a duration or timestamp is just a literal.

I guess that could work ok, although it was always nice to say "Everything that is its own PromQL expression gets its own box in PromLens and can be selected and evaluated/explained individually". Special cases always necessitate special explanations to users and can be confusing. And it'll be easier to handle for the pure tree display case than in-tree editing, where you need to be able to go back and forth between a full expression with children and a simple inlined literal (not sure if I or someone else would get to implementing that in the full PromLens anytime soon TBH).

I'm still slightly worried about missing something else down the line where this could create issues, but that's more of an unknown unknown :)

Nexucis · 2025-02-19T12:14:33Z

if @krajorama you don't want to update the UI, I will handle it no worries

fionaliao · 2025-02-20T16:48:19Z

fyi - I been working on this PR while Krajo is on PTO

Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao · 2025-02-21T16:44:48Z

promql/engine.go

+		panic("parser should have discarded non scalar time expression")
+	}
+
+	// TODO(krajorama): would we ever get annotations here?


Currently this could happen in an extremely contrived case like offset (scalar(limit_ratio(-2, vector(0))))

Note some tests are currently failing (giving confusing error messages). This will be modified in a later commit. Also fixed some linting errors. Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

Using addParseErrf instead of unexpected so the ")" doesn't get reported as the problem. Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao · 2025-02-26T17:59:19Z

promql/parser/generated_parser.y

+                        {
+                            dur, _ := $3.(*ParenExpr)
+                            if dur.Type() != ValueTypeScalar {
+                                yylex.(*parser).addParseErrf($3.PositionRange(), "offset time expression does not evaluate to a scalar")
+                            }
+                            yylex.(*parser).addOffset($1, dur)
                            $$ = $1
                        }


I haven't moved this logic to checkAST(), but I did make a change to the error reporting in 446d0b2

Previously if the value wasn't a scalar, the error would be 1:36: parse error: unexpected ")" in offset, expected time expression does not evaluate to a scalar. The second half of the error message is correct, but it reports the problem being the ). This is because unexpected() logs the last item produced by the lexer. Instead, changed to using addParseErrf(). I'm not 100% happy with that either because the error is reported as parse error: ... but it's a type issue rather than a problem with the structure of the query so it's not really a parse error. I've done it this way because that's how all errors around the parser is being reported, including for type issues with binops in checkAST().

An additional change was to add $$ = $1 to the paren_expr rule. It doesn't seem necessary (because the node is modified in-place in the parser stack, and the stack position that $$ would write to already contains the same modified node), adding this makes the code more consistent with other rules.

krajorama · 2025-02-27T12:22:08Z

if @krajorama you don't want to update the UI, I will handle it no worries

That would be appreciated thank you. Please note that we have changed the syntax a little bit , see #15899 (comment) . We have an event next week at my work, but I'll get back to the implementation after that.

krajorama · 2025-03-25T14:50:43Z

Closing this in favor of #16249

krajorama force-pushed the krajo/duration-arithmetic branch 4 times, most recently from 17831fb to 7f5188e Compare February 3, 2025 12:06

krajorama changed the title ~~feat(promql): duration arithmetics on literals~~ feat(promql): offset from time expressions Feb 3, 2025

krajorama marked this pull request as ready for review February 3, 2025 12:07

krajorama requested a review from roidelapluie as a code owner February 3, 2025 12:07

krajorama requested a review from beorn7 February 3, 2025 12:07

krajorama force-pushed the krajo/duration-arithmetic branch from 7f5188e to 2f144dd Compare February 3, 2025 12:08

beorn7 reviewed Feb 6, 2025

View reviewed changes

fionaliao reviewed Feb 10, 2025

View reviewed changes

krajorama added 2 commits February 11, 2025 17:02

Merge branch 'main' into krajo/duration-arithmetic

623f4e0

updates due to review comments

0efd36e

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

beorn7 reviewed Feb 12, 2025

View reviewed changes

Merge branch 'main' into krajo/duration-arithmetic

74d7d37

Update docs/querying/basics.md

5940b31

Co-authored-by: Björn Rabenstein <beorn@grafana.com> Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao force-pushed the krajo/duration-arithmetic branch from 5f30055 to 5940b31 Compare February 18, 2025 19:07

Add more documentation for time expressions

229652a

Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao force-pushed the krajo/duration-arithmetic branch from 99540bc to 229652a Compare February 20, 2025 16:50

Apply my own review comments

7cd13e2

Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao reviewed Feb 21, 2025

View reviewed changes

fionaliao added 2 commits February 26, 2025 13:56

Add additional error tests

43f6658

Note some tests are currently failing (giving confusing error messages). This will be modified in a later commit. Also fixed some linting errors. Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

Fix parse error for paren_exprs

446d0b2

Using addParseErrf instead of unexpected so the ")" doesn't get reported as the problem. Signed-off-by: Fiona Liao <fiona.liao@grafana.com>

fionaliao force-pushed the krajo/duration-arithmetic branch from de5b98e to 446d0b2 Compare February 26, 2025 15:57

fionaliao reviewed Feb 26, 2025

View reviewed changes

krajorama mentioned this pull request Mar 25, 2025

PromQL: allow arithmetic operations in durations in PromQL parser #16249

Merged

krajorama closed this Mar 25, 2025

krajorama deleted the krajo/duration-arithmetic branch April 10, 2025 07:52

	case *parser.BinaryExpr:
	switch lt, rt := e.LHS.Type(), e.RHS.Type(); {
	case lt == parser.ValueTypeScalar && rt == parser.ValueTypeScalar:
	return ev.rangeEval(ctx, nil, func(v []parser.Value, _ [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, annotations.Annotations) {
	val := scalarBinop(e.Op, v[0].(Vector)[0].F, v[1].(Vector)[0].F)
	return append(enh.Out, Sample{F: val}), nil
	}, e.LHS, e.RHS)

	\| expr OFFSET paren_expr
	{
	dur, _ := $3.(*ParenExpr)
	if dur.Type() != ValueTypeScalar {
	yylex.(*parser).unexpected("offset", "time expression does not evaluate to a scalar")
	}
	yylex.(*parser).addOffset($1, dur)
	}

	// We can only use scalars as timestamp or duration.
	if e.Type() != parser.ValueTypeScalar {
	panic("parser should have discarded non scalar time expression")
	}

Conversation

krajorama commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beorn7 commented Feb 6, 2025

Uh oh!

krajorama commented Feb 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krajorama commented Feb 11, 2025

Uh oh!

beorn7 commented Feb 12, 2025

Uh oh!

beorn7 commented Feb 12, 2025

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fionaliao Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusv commented Feb 12, 2025

Uh oh!

beorn7 commented Feb 13, 2025

Uh oh!

juliusv commented Feb 13, 2025

Uh oh!

beorn7 commented Feb 18, 2025

krajorama commented Jan 29, 2025 •

edited

Loading

fionaliao Feb 19, 2025 •

edited

Loading

fionaliao Feb 21, 2025 •

edited

Loading