Fix a regression when math was used with selectors #8168
Conversation
jsternberg
force-pushed
the
js-8167-math-with-multiple-selectors
branch
from
f010b95
to
4eb1953
Compare
If there were multiple selectors and math, the query engine would mistakenly think it was the only selector in the query and would not match their timestamps. Fixed the query engine to pass whether the selector should be treated as a selector so queries like `max(value) * 1, min(value) * 1` will match the timestamps of the result.
jsternberg
force-pushed
the
js-8167-math-with-multiple-selectors
branch
from
4eb1953
to
b14c292
Compare
|
@rbetts proposed for backporting to 1.2. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return buildRHSTransformIterator(lhs, rhs, expr.Op, b.opt) | ||
| } else if lhs, ok := expr.LHS.(Literal); ok { | ||
| rhs, err := buildExprIterator(expr.RHS, b.ic, b.sources, b.opt, IsSelector(expr.RHS)) | ||
| rhs, err := buildExprIterator(expr.RHS, b.ic, b.sources, b.opt, b.selector) |
There was a problem hiding this comment.
The current implementation of IsSelector:
func IsSelector(expr Expr) bool {
if call, ok := expr.(*Call); ok {
switch call.Name {
case "first", "last", "min", "max", "percentile", "sample", "top", "bottom":
return true
}
}
return false
}
I'm concerned about the other uses of IsSelector being broken in the same way it was here – missing a Call in a binary expression, etc. Is there a separate issue to track/audit the other uses?
There was a problem hiding this comment.
IsSelector wasn't broken here. It was just improperly used. IsSelector correctly identified the expression was a selector, but it was supposed to recognize an earlier conclusion by the query engine not to treat the selectors as selectors.
The reason is because max(), when it's treated as a selector, uses the time of the point it selects. When it's treated as an aggregate, it normalizes the time to be the beginning of the current interval. When we deal with any expression where there are multiple selectors (including binary expressions or separately selected), it's not supposed to treat the selectors as selectors.
There was a problem hiding this comment.
I understand now - IsSelector is working as intended. Fine by me to merge.
If there were multiple selectors and math, the query engine would
mistakenly think it was the only selector in the query and would not
match their timestamps.
Fixed the query engine to pass whether the selector should be treated as
a selector so queries like
max(value) * 1, min(value) * 1will matchthe timestamps of the result.
Fixes #8167.