Mimir query engine: binary operations between two scalars #9277
Conversation
| // | ||
| // So we can just compute the result of each pairwise operation without examining the timestamps of each sample. | ||
| // | ||
| // We store the result in the slice from the left operator, and return the right operator's slice once we're done. |
There was a problem hiding this comment.
I think for safety we should have a check such as len(leftValues) != len(rightValues) and raise an error
There was a problem hiding this comment.
We rely on this fact in other places, and don't have checks there either (eg. scalar / vector binary operations).
If the lengths are different, the code will panic, which seems like a reasonable thing to do for a bug where an invariant has been broken.
There was a problem hiding this comment.
It only panics if the right side is shorter than the left. I don't think the overhead here is very high, what do you think about it adding it to the other scalar operators too?
There was a problem hiding this comment.
I'm not sure this makes sense as a precedent to set: we don't do similar validation-type checks in other situations (eg. checking that the data points from an instant vector operator are within the query time range, or align to the requested step), and if those situations were to occur, we'd have similarly undefined behaviour.
There was a problem hiding this comment.
Hmm, okay. I still think it's reasonable, but I don't hold it strongly. LGTM :-)
There were existing tests that exercised |
| // | ||
| // So we can just compute the result of each pairwise operation without examining the timestamps of each sample. | ||
| // | ||
| // We store the result in the slice from the left operator, and return the right operator's slice once we're done. |
There was a problem hiding this comment.
Hmm, okay. I still think it's reasonable, but I don't hold it strongly. LGTM :-)
What this PR does
This PR adds support for binary operations between two scalars in MQE.
For example, expressions like
2 + 3andscalar(metric) * 2are now supported.Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.