promql: add limitk aggregation operator (#88) by petrpan26 · Pull Request #442 · opendata-oss/opendata

petrpan26 · 2026-05-10T06:08:10Z

Summary

Adds evaluator support for limitk with Prometheus-compatible grouping and filter-shape output.

add limitk aggregation reusing the topk/bottomk per-group heap as a push-only buffer
walk inputs in fingerprint-sorted order for stable selection
extend lowering and physical planning to recognize limitk
add limitk-focused operator tests
add limitk parity fixtures to aggregators.test

Related Issues

Fixes #88

Test Plan

cargo test -p opendata-timeseries --lib
cargo clippy -p opendata-timeseries --lib --all-targets -- -D warnings

Checklist

Tests added/updated
cargo fmt and cargo clippy pass

petrpan26 · 2026-05-10T06:11:47Z

Added a draft PR here for review purposes + testing blocking on GreptimeTeam/promql-parser#142 for a new release so we use an official version.

v0.9.0 adds grammar rules for limitk and limit_ratio in the aggregate_op production. v0.8.0 declared these as `%expect-unused` tokens but never bound them in the grammar, so `limitk(...)` queries failed at parse time before this bump. The bump also widens the token id type from u8 to u16. Updated two local helper signatures (aggregate_op_name, binary_op_kind) to match. No behavior change.

cadonna

Thanks for the PR, @petrpan26 !

I reviewed the production code and had a couple of comments.

I am aware the some comments are not directly related to your code.

cadonna · 2026-05-12T10:35:34Z

+                    continue;
+                }
+                // KOrder is unused — limitk never evicts.
+                heap.push(KHeapEntry {


Why do we need to first push the series samples to a heap and then from the heap to out? Why can we not set the samples directly in out?

Hi I think I got a very bad case of dumb ai coding in this PR. Upstreams code were reusing the same allocated heap for topk and bottomk and limitk. And i think i blindly follow it. I think our code structure is completely different.

Leave this here so you can actually review the new logic

This PR adds first-class evaluator support for the limitk aggregation. It hooks AggregateOp::Limitk into the aggregation evaluation path with Prometheus-compatible grouping and filter-shape output. - implement limitk via AggregateKind::Limitk(i64), reusing the topk/bottomk per-group heap as a fixed-capacity push-only buffer (no eviction) - walk inputs in fingerprint-sorted order so selection is stable per query (Blake3 fingerprint already on SeriesSchema; diverges from upstream's xxhash) - extend lower_aggregate's shared K-param block to T_TOPK | T_BOTTOMK | T_LIMITK - extend physical-plan filter-shape to include Limitk - add limitk-focused operator tests covering by/without grouping, k=0/-1/k>n, mixed validity, multi-step independent selection, fingerprint-sort correctness, and out-of-order tile arrival - add limitk parity fixtures in aggregators.test (cardinality assertions; full label assertions where output is fully determined)

petrpan26 · 2026-05-12T13:44:54Z

@cadonna Thanks for the review I think I was too enthusiastic when upstream deps was merged and forgot to do an actual code sweep.

apurvam · 2026-05-13T04:06:58Z

Hi @petrpan26 . This looks good. I think the only thing that's missing is to add a memory reservation, like the topk functions do. See should_respect_memory_reservation_for_topk_heap for a test that throws a MemoryLimit error when the topk function exceeds it's memory reservation.

TBH, I'm not exactly sure how this memory limits work, but it's better to be consistent here IMO.

cadonna · 2026-05-13T08:16:28Z

Hi @petrpan26 . This looks good. I think the only thing that's missing is to add a memory reservation, like the topk functions do. See should_respect_memory_reservation_for_topk_heap for a test that throws a MemoryLimit error when the topk function exceeds it's memory reservation.

TBH, I'm not exactly sure how this memory limits work, but it's better to be consistent here IMO.

I agree that a test is needed for the memory reservation. However, as limitk does not use a heap, it should only test for the reservation of the grid (line 670 in aggregates.rs). Additionally, as far as I can see test should_respect_memory_reservation_for_topk_heap() does not test what is suggests it tests. The verified error is caused by the grid reservation, not by the heap reservation (line 698 in aggregates.rs). Ideally, we have the following tests:

for each of topk, bottomk, limitk, and quantile one test where the reservation of the grid exceeds the cap
for each of topk, bottomk one test where the heap exceeds the cap
for quantile one test where the sort buffer exceeds the cap

@petrpan26 if you have cycles, you are welcome to fix that in a separate PR.

cadonna

Thanks for the updates @petrpan26 !

The algorithm looks much better now.

I have one minor comment for the algorithm and two comments for tests.

cadonna · 2026-05-13T07:17:11Z

+            let grid_base = global_step * in_series_count;
+            let out_base = global_step * in_series_count;


grid_base and out_base are always equal, right?
Same question about out_idx and cell.
If yes, you can just use grid_base and cell.

cadonna · 2026-05-13T08:25:45Z

 	http_requests{group="canary", instance="0", job="app-server"} 700
 	http_requests{group="production", instance="1", job="app-server"} 600

+# Tests for limitk.


For these tests did you also consider the tests here: https://github.com/prometheus/prometheus/blob/main/promql/promqltest/testdata/limit.test ?

cadonna · 2026-05-26T08:37:37Z

@petrpan26 do you plan to update this PR?
If not, we can merge as is and I can include my feedback on a separate PR.
Just let us know. Either way is fine. We appreciate your contribution.

petrpan26 · 2026-05-26T10:44:18Z

@cadonna im afk this week. Do you mind if i take this on this weekend. If this block anything pls feel free to merge it

cadonna · 2026-05-26T11:22:27Z

@petrpan26 It is not blocking anything. I just wanted to understand if the PR is still on your radar. Great that you plan to work on the PR!

petrpan26 · 2026-06-01T17:10:15Z

Looking into this tomorrow !

petrpan26 changed the title ~~promql: add limitk aggregation operator (#88)~~ [WIP] promql: add limitk aggregation operator (#88) May 10, 2026

petrpan26 force-pushed the feat/limitk branch from 448cea8 to 4683044 Compare May 10, 2026 06:15

petrpan26 changed the title ~~[WIP] promql: add limitk aggregation operator (#88)~~ promql: add limitk aggregation operator (#88) May 10, 2026

petrpan26 force-pushed the feat/limitk branch 2 times, most recently from b9dc872 to 7a19b1a Compare May 10, 2026 06:42

petrpan26 force-pushed the feat/limitk branch from 7a19b1a to 2e32687 Compare May 12, 2026 09:21

petrpan26 mentioned this pull request May 12, 2026

promql: add limit_ratio aggregation operator #443

Draft

3 tasks

cadonna reviewed May 12, 2026

View reviewed changes

petrpan26 force-pushed the feat/limitk branch from 2e32687 to 56a0e6f Compare May 12, 2026 13:09

petrpan26 force-pushed the feat/limitk branch from 56a0e6f to 24b944b Compare May 12, 2026 13:23

petrpan26 requested a review from cadonna May 12, 2026 13:46

cadonna reviewed May 13, 2026

View reviewed changes

Merge branch 'main' into feat/limitk

bd4a56b

		let grid_base = global_step * in_series_count;
		let out_base = global_step * in_series_count;

Uh oh!

Conversation

petrpan26 commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issues

Test Plan

Checklist

Uh oh!

petrpan26 commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cadonna left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cadonna May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrpan26 May 12, 2026

Choose a reason for hiding this comment

Uh oh!

petrpan26 May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrpan26 commented May 12, 2026

Uh oh!

apurvam commented May 13, 2026

Uh oh!

cadonna commented May 13, 2026

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

cadonna May 13, 2026

Choose a reason for hiding this comment

Uh oh!

cadonna May 13, 2026

Choose a reason for hiding this comment

Uh oh!

cadonna commented May 26, 2026

Uh oh!

petrpan26 commented May 26, 2026

Uh oh!

cadonna commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrpan26 commented Jun 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

petrpan26 commented May 10, 2026 •

edited

Loading

petrpan26 commented May 10, 2026 •

edited

Loading

cadonna left a comment •

edited

Loading

cadonna May 12, 2026 •

edited

Loading

cadonna commented May 26, 2026 •

edited

Loading