Mimir Query Engine: Add instant-vector functions #8256
Conversation
jhesketh
changed the title
To make sure we can handle scalar arguments Also tidy up a few things for consistency
The expression won't error, but the results still need work.
| // Most of the functionality of functions is tested through the test scripts in | ||
| // pkg/streamingpromql/testdata. | ||
|
|
||
| func TestFunctionOverInstantVector(t *testing.T) { |
There was a problem hiding this comment.
What is this test trying to test? I'm not sure this is necessary - we can rely on the tests in the test scripts for the functions themselves.
There was a problem hiding this comment.
It doesn't test anything extra that wouldn't be covered by the functional tests, but it is a unit test to confirm it does what we expect. It tests two things:
- That both the
MetadataFuncandSeriesDataFuncare called - That
SeriesDataFuncis called once per series
There was a problem hiding this comment.
I still don't think this is necessary - we can confirm that the functions are called by observing their effects (eg. that the label name has been dropped or that the values returned are equal to acos(<value>)).
There was a problem hiding this comment.
Whilst very unlikely, it is possible that we confirm the effects correctly but do something like call SeriesDataFunc more than once.
We could make sure that is unlikely by writing functional tests (ie the test scripts) for functions that are non-idempotent, but then we run the risk of not maintaining that test correctly (eg we refactor or remove it in the future).
I agree it's very unlikely (maybe even impossible), and that this test (and the others) don't necessarily add much, but I also don't see it as a huge burden to keep the extra testing and assurances.
Worst case in regards to these (likely redundant) tests, is that we remove them later when we need to change the way something works internally and still satisfies the existing functional test.
Also provide some more generic helper functions.
| @@ -19,7 +19,7 @@ | |||
| * [FEATURE] Continuous-test: now runable as a module with `mimir -target=continuous-test`. #7747 | |||
| * [FEATURE] Store-gateway: Allow specific tenants to be enabled or disabled via `-store-gateway.enabled-tenants` or `-store-gateway.disabled-tenants` CLI flags or their corresponding YAML settings. #7653 | |||
| * [FEATURE] New `-<prefix>.s3.bucket-lookup-type` flag configures lookup style type, used to access bucket in s3 compatible providers. #7684 | |||
| * [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.promql-engine=mimir`. #7693 #7898 #7899 #8023 #8058 #8096 #8121 #8197 #8230 #8247 #8270 #8276 #8277 #8291 #8303 #8340 | |||
| * [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.promql-engine=mimir`. #7693 #7898 #7899 #8023 #8058 #8096 #8121 #8197 #8230 #8247 #8270 #8276 #8277 #8291 #8303 #8340 #8256 | |||
There was a problem hiding this comment.
[nit] Would be good to keep the PRs sorted
There was a problem hiding this comment.
Depends if you want them sorted by order merged, or order opened. I'd argue for merged since this branch merges main back into itself and will be squashed and applied on top of what was 8340.
| // Most of the functionality of functions is tested through the test scripts in | ||
| // pkg/streamingpromql/testdata. | ||
|
|
||
| func TestFunctionOverInstantVector(t *testing.T) { |
There was a problem hiding this comment.
I still don't think this is necessary - we can confirm that the functions are called by observing their effects (eg. that the label name has been dropped or that the values returned are equal to acos(<value>)).
Start to add instant-vector functions.
histogram_countandhistogram_sumacosThis is just the start to show the general structure. I'd like to get this approach verified and merged and then add the rest of the simpleFunc's (such as the basic maths operations) as a separate PR.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.