Evaluate PromQL topk/bottomk/limitk with a streaming O(T * k) plan - #113656
Evaluate PromQL topk/bottomk/limitk with a streaming O(T * k) plan#113656nikitamikhaylov wants to merge 2 commits into
Conversation
|
Workflow [PR], commit [3231ae1] Summary: ❌
AI ReviewSummaryThis PR replaces the old PromQL Findings
Tests
Final VerdictChanges requested. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 376/390 (96.41%) · Uncovered code |
|
📊 Cloud Performance Report ✅ AI verdict: This change is entirely scoped to experimental PromQL topk/bottomk/limitk streaming evaluation: new timeSeriesSelect*Groups aggregate functions (gated behind experimental settings) plus the TimeSeries query-plan builder. None of the flagged ClickBench (Q17, Q28, Q32) or TPC-H (Q4, Q7) queries execute that code path, so none of the deltas can plausibly come from this PR. Q17's flagged +11.9% is therefore downgraded to not_sure as run-to-run variance, and the TPC-H Q4/Q7 envelope suppressions are consistent with the data since both source medians fall inside master's current variance band. No cross-query operator pattern is present; these are independent off-path fluctuations. clickbenchFlagged queries (3 of 43)
Change = percent below ×2; the ratio of medians (×N faster/slower) beyond, where percent understates the scale. q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. tpch_adapted_1_officialFlagged queries (2 of 22)
Change = percent below ×2; the ratio of medians (×N faster/slower) beyond, where percent understates the scale. q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. Debug info
|
The AI review found that deserialize rejected states with more than 1'000'000 time steps while add and serialize enforced no such limit, so a valid state built for a longer range could be produced and then fail with an exception only when it was merged across threads or shards or read back from a table. deserialize now grows the state incrementally while reading, so the allocated memory is bounded by the actual payload and corrupted data claiming a huge size fails with an end-of-buffer error instead of attempting a huge upfront allocation. The heap_size <= k invariant check remains. The test now round-trips a state with more than a million time steps through a MergeTree table and covers both corrupted-state rejections. Review: #113656 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@vitlibar I tried to understand whether we can change the original
|
The previous plan collected all N series into a single row and selected
per-step winners with arrayTopK, whose lambda capture replicated an
N-element array per element: O(T * N^2) memory in one thread. On real
data (N = 25600) it attempted allocations of 147.71 GiB (T = 121) and
3.43 TiB (T = 2880).
The new aggregate functions timeSeriesSelect{TopK,BottomK,LimitK}Groups
keep one bounded heap of size k per time step (deterministic ties by
smaller group) and return per-series masks of the selected steps, which
are joined back onto the streaming per-series grid rows. The 3000-series
by 200-step regression test now runs with a 91.51 MiB peak instead of
attempting a 13.41 GiB allocation, with byte-identical results.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The AI review found that deserialize rejected states with more than 1'000'000 time steps while add and serialize enforced no such limit, so a valid state built for a longer range could be produced and then fail with an exception only when it was merged across threads or shards or read back from a table. deserialize now grows the state incrementally while reading, so the allocated memory is bounded by the actual payload and corrupted data claiming a huge size fails with an end-of-buffer error instead of attempting a huge upfront allocation. The heap_size <= k invariant check remains. The test now round-trips a state with more than a million time steps through a MergeTree table and covers both corrupted-state rejections. Review: #113656 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fa6a2f7 to
3231ae1
Compare
| group_data.push_back(group); | ||
|
|
||
| const size_t mask_begin = mask_data.size(); | ||
| mask_data.resize_fill(mask_begin + num_steps, 0); |
There was a problem hiding this comment.
insertResultInto re-expands the bounded heap state into one full steps_mask per distinct selected series, so the selected_groups cell becomes O(num_steps * distinct_selected_groups), not O(num_steps * k). In the churny case where different series win at different time steps, distinct_selected_groups grows to min(num_series, num_steps * k), and this line allocates a UInt8 mask of length num_steps for each of them.
That means the rewritten PromQL plan can still materialize a very large single-row intermediate before the arrayJoin/join-back phase. With the PR's own 30-day / 1-second example, num_steps = 2_592_000; if the winners churn enough to touch all 25,600 series over the range, this path alone builds about 61.8 GiB of masks here. The existing memory regression test keeps the same winners at every step (value = id), so it does not exercise this case.
I think the selection result needs to stay step-oriented through the join-back, or at least this needs a churny memory regression test before we claim the new plan is bounded by O(T * k).
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Evaluate the PromQL aggregation operators
topk,bottomkandlimitkwith a streaming plan whose selection state isO(time_steps * k)instead of collecting all series into a single row withO(time_steps * series^2)intermediate memory. Queries over high-cardinality metrics that previously failed withMEMORY_LIMIT_EXCEEDEDnow run in bounded memory, and the evaluation is parallelized instead of single-threaded.Description
Closes #112335.
The transpiler evaluated
topk/bottomk/limitkby collapsing the whole per-series grid into a single row (arrayTranspose(groupArray(values))) and selecting per step witharrayMap(v -> arraySort(arrayTopK(i -> v[i], k, arrayEnumerate(v))), values). The lambda capturesv(an N-element array) and captured columns are replicated per array element, so one time step materializes an N x N matrix —O(T * N^2)memory independent ofk, all in one row processed by one thread. On a TimeSeries table with 62.5 billion samples,topk(10, sum by(ns, pod)(rate(...)))over 25,600 series attempted a 147.71 GiB allocation for a 30-minute range and 3.43 TiB for a 30-day range.New experimental aggregate functions (gated like the rest of the
timeSeries*family) select in streaming fashion with one bounded heap of(rank, group)per time step —O(T * k)state, ~450 KiB for T = 2,880, k = 10 — and deterministic tie-breaking (smaller group wins; the old plan's tie-break was arrival-order-dependent):timeSeriesSelectTopKGroups(group, values, k)/timeSeriesSelectBottomKGroups(...)/timeSeriesSelectLimitKGroups(..., sampling_key)kmay be a literal, a scalar subquery, or anArray(UInt64)with one value per step (time-dependent PromQLk). The result is per selected series a per-step 0/1 mask, and the plan becomes:The grid is evaluated twice (selection + masking pass) — a deliberate recompute-for-memory trade; every stage streams in parallel. NULL cells are never candidates and NaN loses to any non-NaN value, matching
arrayTopK/arrayBottomKand Prometheus. Exactly k series per step, NULLs where a series is not selected — byte-compatible with the old masking semantics. Serialized states carry a format version and validate wire data (size caps, per-step count <= k).Measured on the 62.5-billion-sample table (all other query fingerprints unchanged):
topk(10, sum by(ns,pod)(rate(...))), 30 m rangeThe 30-day peak is the underlying
rategrid aggregation state (identical to a plain 30-daysum(rate(...))on this table); the selection state itself is ~450 KiB.Verification: 17 end-to-end
topk/bottomk/limitkqueries (range + instant,by/without, k > N, k = 0, negative k, per-step k, scalar-subquery k, NaN, no-match metric) are byte-identical to the base binary; results are deterministic acrossmax_threads1/4/16 including a tie-heavy stress; all PromQL stateless tests pass with both analyzers.Tests: a new stateless test covering the aggregate functions directly (ties, NaN, NULL gaps, serialization round-trip, corrupted-state rejection, argument validation) and the operators end to end, including a memory regression —
topk(10, ...)over 3,000 series x 200 steps undermax_memory_usage = 2 GB, where the old plan attempts a single 13.41 GiB allocation and the new plan peaks at 91.5 MiB — plustests/performance/promql_topk.xml(4,000 series, 300-step ranges).🤖 Generated with Claude Code