Repository navigation
test(query-engine): cover temporal anchors and topk ordering - #775
milindsrivastava1997 merged 3 commits into
Conversation
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Code review: 3 findings.
| } | ||
| return metricString(labels) | ||
| } | ||
| excluded := make(map[model.LabelName]struct{}, len(grouping.Labels)) |
There was a problem hiding this comment.
without grouping keeps __name__, unlike Prometheus. Prometheus always drops __name__ from the grouping key for without, and topk/bottomk keep __name__ in their output, so a multi-metric selector gets split into per-name groups here.
Example: topk without (instance) (3, {__name__=~"a|b"}) → reference returns one group ordered a{i1}=5, b{i1}=4, a{i2}=3; the checker reports group "…a…" is not contiguous against a correct result. Conversely, a wrong cross-name order passes, since order is only checked within each name.
Suggest always excluding __name__ in the without branch.
| name: topk-ordering | ||
|
|
||
| queries: | ||
| # Equal values make the full label set the TopK tie-breaker. |
There was a problem hiding this comment.
These cases can't catch a tie-breaking bug. topk(6, …) runs over exactly 6 series, and the by (job) / without (instance) cases with k=3 run over exactly 3 series per group, so every series is always selected and the tie-breaker never decides membership. compareInstantValues also doesn't check the order of tied samples. An engine that breaks ties differently would pass. Using k smaller than the group size would make these meaningful.
| series: | ||
| - metric: ordered_data | ||
| labels: {job: backend, instance: a} | ||
| samples: [{offset_seconds: 0, value: 1}, {offset_seconds: 300, value: 1}, {offset_seconds: 600, value: 1}] |
There was a problem hiding this comment.
Samples end exactly at the last evaluated offset (600). aggregations.yaml deliberately adds a sample past the last evaluated offset so the trailing window has a real later sample to close on, rather than only the wall-clock idle fallback. Without one, the 600s evaluations here rely on that fallback and may be flaky in the ASAP engine. Consider adding a sample at e.g. 660.
Triage outcome for the three findings:
|
Summary
Extends PromQL differential coverage for temporal functions and adds an opt-in instant-vector ordering policy for TopK/BottomK.
Test rules: before
/query,/query_range, and range-at-t/instant-at-t parity therefore compared result membership and values, but never response presentation order.rate,increase,sum_over_time, andcount_over_time, plus selected direct aggregations.Test rules: after
comparison.instant_vector_orderwithascendingordescending.by/withoutTopK/BottomK, each bucket must be contiguous and ordered internally; bucket order is intentionally unconstrained./query_rangeand range-at-t/instant-at-t parity remain order-insensitive.Coverage added
min_over_time,max_over_time, and validsum/count/min/maxtemporal-reducer compositions across ungrouped,by, andwithoutgrouping.by (job), andwithout (instance)behavior.Docker compliance status
single-rate-temporalsingle-rate-off-grid-rateaggregations-native-dagaggregationsavg_over_time)topk-orderingby/withoutexpose bucket interleavingquantilesolly-benchVerification
go test ./...inpromql-compliance/runnertopk-orderingcompliance run: ungrouped tied TopK passes; grouped cases expose the current engine bucket-interleaving bug.Known follow-up work
avg_over_timeand several nested/rate shapes currently returnNo result for query; nested temporal pipeline support is tracked in feat(planner): support nested temporal aggregation pipelines #772.