MQE: Implement absent_over_time #10841
Conversation
pkg/streamingpromql/engine_test.go
Outdated
| unsupportedExpressions := map[string]string{ | ||
| "absent_over_time(nonexistent{}[1h])": "'absent_over_time' function", | ||
| } | ||
| unsupportedExpressions := map[string]string{} |
There was a problem hiding this comment.
Should we add holt_winters here until we add support for that? Or perhaps an experimental function?
There was a problem hiding this comment.
Updated in d646f61. Let me know if something is incorrect there.
| for { | ||
| step, err := a.inner.NextStepSamples() | ||
| // nolint:errorlint // errors.Is introduces a performance overhead, and NextStepSamples is guaranteed to return exactly EOS, never a wrapped error. | ||
| if err == types.EOS { | ||
| break |
There was a problem hiding this comment.
We could simplify this slightly by changing the for to something like for stepIdx := range a.timeRange.StepCount. We can then drop the special check for err == types.EOS (we should never receive an EOS for any of the steps, and if we do, we should return an error) and line 82 then doesn't need to call PointIndex.
|
|
||
| var err error | ||
| for step := range a.timeRange.StepCount { | ||
| t := a.timeRange.IndexTime(int64(step)) |
There was a problem hiding this comment.
[nit] We only need to call IndexTime if we're going to emit a sample for this step, so this could move down beneath the if
| timeRange types.QueryTimeRange | ||
| argExpressions parser.Expr | ||
| inner types.RangeVectorOperator | ||
| expressionPosition posrange.PositionRange | ||
| memoryConsumptionTracker *limiting.MemoryConsumptionTracker | ||
| presence []bool | ||
| exhausted bool |
There was a problem hiding this comment.
[nit] We usually export some of these - could you please make this consistent with the other operators? The usual rule of thumb is that internal implementation details aren't exported.
|
Could you please also add this to #10067? |
|
@charleskorn all of your PR feedbacks has already been addressed. Please take another look. |
| // expected error for holt_winters is the same as double_exponential_smoothing because mimir just aliasing it as above | ||
| "holt_winters(metric{}[1h], 1, 1)": "'double_exponential_smoothing' function", | ||
|
|
||
| "double_exponential_smoothing(metric{}[1h], 1, 1)": "'double_exponential_smoothing' function", |
There was a problem hiding this comment.
I think adding just double_exponential_smoothing here is sufficient - no need to add holt_winters (and the aliasing above) as well.
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
f48e01e to
54f36f4
Compare
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.