promql: Fix unary operator precedence in duration expressions #16713
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
0d1e0b7 to
2943a85
Compare
2943a85 to
6dfe4d7
Compare
Split unary operator handling in duration expressions into two specific cases to fix precedence conflicts: - Handle unary operators with number literals directly - Handle unary operators with parenthesized expressions separately This prevents unary minus from incorrectly binding to subsequent operators in expressions like `foo offset -1^1`, ensuring it parses as `(foo offset -1) ^ 1` rather than `foo offset (-1^1)`. Fixes prometheus#16711 Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
6dfe4d7 to
6d56e77
Compare
|
Will have a look ASAP, but @krajorama is probably way more qualified for this anyway. |
This was referenced Jun 15, 2025
MichaHoffmann
approved these changes
Jun 15, 2025
There was a problem hiding this comment.
LGTM! this also fixes a crash in the parser related to offset arithmetic on this expression:
avg(
{__name__="http_requests_total",route!="/"} offset -1m
^
{__name__="http_requests_total",route!="/"}
)
which crashes without this commit on
time=2025-06-15T09:13:36.500Z level=INFO source=manager.go:176 msg="Starting rule manager..." component="rule manager"
parser panic: runtime error: index out of range [0] with length 0
goroutine 722 [running]:
github.com/prometheus/prometheus/promql/parser.(*parser).recover(0xc0004b2fc0?, 0xc001574968)
/home/mhoffmann/git/upstream/prometheus/promql/parser/parse.go:343 +0x99
panic({0x3cb8000?, 0xc001680270?})
/nix/store/02kl9hh07rl3iyvsdrcrhgq7xzi0wf2w-go-1.23.8/share/go/src/runtime/panic.go:791 +0x132
github.com/prometheus/prometheus/promql/parser.(*parser).newAggregateExpr(0xc000a78608?, {0x40583f9?, 0xb?, {0xc00156c005?, 0x0?}}, {0x46bcc20?, 0xc000102ae0?}, {0x46bcd40?, 0xc001574878?})
/home/mhoffmann/git/upstream/prometheus/promql/parser/parse.go:456 +0x354
github.com/prometheus/prometheus/promql/parser.(*yyParserImpl).Parse(0xc000a786a8, {0x46a4fe0, 0xc000a78608})
but works fine on this branch!
krajorama
reviewed
Jun 16, 2025
There was a problem hiding this comment.
Makes sense. I'd add something in the doc https://prometheus.io/docs/prometheus/latest/feature_flags/#promql-arithmetic-expressions-in-time-durations about this case and mention that if in doubt use ().
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
krajorama
approved these changes
Jun 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split unary operator handling in duration expressions into two specific cases to fix precedence conflicts:
This prevents unary minus from incorrectly binding to subsequent operators in expressions like
foo offset -1^1, ensuring it parses as(foo offset -1) ^ 1rather thanfoo offset (-1^1).Fixes #16711