[PERF] PromQL: Move more work to preprocessing step #16896
Conversation
This should mean we can stop unwrapping them later. Fix up tests that check very specific details. Signed-off-by: Bryan Boreham <[email protected]>
Matrix selectors have a Timestamp which indicates they are time-invariant, so we don't need to wrap and then unwrap them when we come to use them. Fix up tests that check this level of detail. Signed-off-by: Bryan Boreham <[email protected]>
In aggregations and function calls. We no longer wrap the literal values or matrix selectors that would appear here. Signed-off-by: Bryan Boreham <[email protected]>
This means we only do it once, rather than on every step of a range query. Also the code gets a bit shorter. Signed-off-by: Bryan Boreham <[email protected]>
|
|
||
| case *parser.MatrixSelector: | ||
| return preprocessExprHelper(n.VectorSelector, start, end) | ||
| // We don't need to wrap a MatrixSelector because functions over range vectors evaluate those directly, | ||
| // and they can't appear at top level in a range query. |
There was a problem hiding this comment.
On second thoughts, I think this second line of comment is unnecessary.
It was provoked by a test failing.
So long as all evaluations of MatrixSelector check the Timestamp field, we don't need a wrapper.
There was a problem hiding this comment.
i think this would change after #14095? post that PR, the MatrixSelector can appear in a range query and it will be evaluated in an eval call, so we'd want it to be wrapped in StepInvariantExpr if it is indeed step invariant
eg:
rate(my_metric[5m] @ start() != 0)
There was a problem hiding this comment.
One thing that perhaps isn't obvious about preprocessExprHelper is that it is coded to wrap the highest possible node in the tree with StepInvariantExpr. In your example the BinaryExpr node should get wrapped, and no wrapping is needed on the MatrixSelector.
Try it!
| @@ -2304,20 +2304,16 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) { | |||
| }{ | |||
| { | |||
| input: "123.4567", | |||
There was a problem hiding this comment.
I think there is still one more missing unwrap ...
This can be verified by adding in test cases for the following;
- "()"
("<string>")("")
Currently these would not be unwrapped to the NumberLiteral and StringLiteral.
I think in the PreprocessExpr() it is safe to do a unwrapParenExpr(&expr) before passing into the preprocessExprHelper and this will then allow the above test cases to pass.
The current flow of handling these cases in the *parser.ParenExpr in the preprocessExprHelper() does not actually strip the unnecessary parent expr.
Try adding these test cases in;
{ input: "(123.4567)", expected: &parser.NumberLiteral{ Val: 123.4567, PosRange: posrange.PositionRange{Start: 1, End: 9}, }, }, { input:("foo"), expected: &parser.StringLiteral{ Val: "foo", PosRange: posrange.PositionRange{Start: 1, End: 6}, }, }, { input: (""), expected: &parser.StringLiteral{ Val: "", PosRange: posrange.PositionRange{Start: 1, End: 3}, }, }
There was a problem hiding this comment.
My PR does not aim to unwrap those expressions. They evaluate correctly, right? That's all we need.
There was a problem hiding this comment.
("") and ("") work. I double checked one of the originals range queries in unecessary () and it is also good.
() returns an error, but I am guessing that is not a valid query.
So all good!
|
hey @bboreham - I think there is one more case to be considered which can be exposed by adding another few test cases to the TestPreprocessAndWrapWithStepInvariantExpr() - see comment on the file. |
Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: Bryan Boreham <[email protected]>
Instead of doing lots of unwrapping during the evaluation phase, take more care over what we wrap, and unwrap superfluous parens that the caller added, during the preprocessing step.
This means we only do it once, rather than on every step of a range query. Also the code gets a bit shorter.
Looking at the history, some of this came in #9591 as a fix for #9590, because the original version was too eager to unwrap. This version does not unwrap parens on binary expressions, where they are important for precedence.
Matrix selectors have a Timestamp which indicates they are time-invariant, so we don't need to wrap and then unwrap them when we come to use them.
Fix up tests that check very specific details which change due to this change of approach.
This should address #16880, although I didn't add any tests for that (yet).
Separated into multiple commits for ease of understanding.
There is one remaining call to
unwrapStepInvariantExpr, in the special handling fortimestep(). This seems like a bad smell, but I left it for another time.