Add check to ensure we never parse multiple range intervals when mapping an instant query for splitting

[pracucci]

What this PR does

getRangeInterval() makes me uncomfortable because it returns the 1st range interval found. Theoretically, due to how we use it, we should never call it with an expr that could have 2+ matrix selectors, but it's not enforced in the code.

In this PR I'm enforcing it, adding a check that will fail the mapping if we call it with an expr containing 2+ matrix selectors.

Which issue(s) this PR fixes or relates to

N/A

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[ssncferreira]

Thanks, the idea looks good, but I have a concern with Subqueries 🤔

[ssncferreira]

If I understand correctly, this will return an empty array if expr is a subquery right?
If so, can we add a test to TestGetRangeIntervals?

[ssncferreira]

From looking at Suquery it can have an expression: https://github.com/prometheus/prometheus/blob/main/promql/parser/ast.go#L131-L146
so that means that we can have something like sum_over_time(rate(metric[1m])[5h:5m]), right?
In this case, I think this will return [1m] and it cannot be split 🤔

[pracucci]

That's a good point! I think the utilities should be agnostic, but we should avoid splitting subqueries in the caller. I'm proposing it here: 37076dd

We do something similar (but a bit more evoluted to support some subqueries) in query sharding:

mimir/pkg/frontend/querymiddleware/astmapper/sharding.go

Line 95 in cc0ef42

if isSubquery(e) {

[colega]

Can you explain what is this checking?

I see this function checks whether the first arg of a call is a subquery, but I don't understand why are we checking that (why only the first arg? why don't we just dig recursively?)

[pracucci]

I inherited this logic from the query sharding. See here:
mimir/pkg/frontend/querymiddleware/astmapper/sharding.go

In PromQL functions I don't think we can have functions where a subquery in a position different than 0. Can we?

[colega]

histogram_quantile has the vector as the second argument

[colega]

Oh, but it has to be a range aggregation call. Well, still I think it's worth fixing the function to be future-proof instead of taking those assumptions. But I see the point now. Thanks.

[pracucci]

Actually quantile_over_time(scalar, range-vector) takes the range-vector as 2nd parameter. I'm currently trying to write a test to break the current logic and then I will fix it.

[pracucci]

Fixed here:
1e88552

[ssncferreira]

LGTM 🍪

[ssncferreira]

LGTM, let's try it 💥
Just a couple of comments, but non-blocking

[pracucci]

Wasn't able to find a way for an inner expression in Call to be a BinaryExpr 🕵️‍♀️
Do you know if it is possible?

@ssncferreira BinaryExpr can only have scalar or instant vectors on its legs. A subquery returns a range vector, so you can't use directly as a leg in a BinaryExpr. For example, this is invalid: up[5m:1m] + 1. What you can do is wrap the subquery with an aggregation function (e.g. rate(up{namespace="cortex-dev-01"}[5m:1m]) + 1) but then it's a function call the parent of the subquery, so we enter in the logic added in this PR.

[pracucci]

🤞