Repository navigation
fix(planner): heap top-k plans compile and answer per ranked item - #831
Merged
Merged
Conversation
This was referenced Oct 21, 2026
zzylol
force-pushed
the
fix/fused-topk-heap-executor
branch
from
October 21, 2026 20:08
e6f64a4 to
886c264
Compare
A heap's SummaryEstimate{TopK} declared one Utf8 row per partition, so
the executor's keyed evaluation (k rows per partition, then the item
columns and a Float64 estimate) refused it, and Stage 3's compile check
rejected every heap top-k plan.
- The estimate's schema is the keyed evaluation's: the partition
columns, the build's item columns, then the Float64 estimate.
- Pass 1 projects it to the top-k's [keys.., topk_k], then (over PromQL
rows) the item columns, as the exact top-k's physical plan answers.
- The fused heap over a counter derivative is offered again: PromQL rows
carrying the series identity are closed, but their group labels are
item columns.
- Stage 3 compiles the plan with its weight requirements confirmed, the
plan the executor runs.
…ic name The fused heap ranks the raw samples' series identities, which carry __name__; the sum_over_time or count_over_time it answers drops it, as the exact top-k's answers do. Its estimate now drops the name like every other temporal summary evaluation, so the answers match the exact plan's label sets.
zzylol
force-pushed
the
fix/fused-topk-heap-executor
branch
from
October 21, 2026 20:24
886c264 to
78874ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
No PromQL top-k could ever use a heap sketch. The fused whole-expression heap (#692) is the only heap Pass 1 still offers for a PromQL top-k (#805 pruned the others). Its
SummaryEstimate{TopK}declared one Utf8topkvalue per partition. The executor's keyed evaluation returnskrows per partition: the partition columns, the ranked item, and a Float64 estimate. So the executor refused every such plan, and since #805 Stage 3's compile check rejected all of them (ExecutorCannotCompile). The ASAPQuery top-k-sum queries (q06-topk-sum,topk by (region, service) (4, sum_over_time(data[1m]))) could not pick the cheaper heap plan even with guarantees ignored.What
A heap top-k plan now compiles, and it answers
krows per partition: the right group labels, the ranked series (or group), and its estimated value. This covers:topk by (G) (k, sum_over_time(m[W]))andtopk by (G) (k, count_over_time(m[W])): one heap per partition over the raw samples, ranking series identities;topk(k, sum by (G) (increase(m[W])))and the same withrate, over counters: one heap over the exact per-series values, ranking theGgroups.Each answer has the exact top-k's physical shape,
[keys.., topk_k, item labels..]. This is the same shape #794/#805 gave exact top-k answers. A range function's result has no__name__, as in Prometheus.Not covered:
topk(k, sum by (G) (sum_over_time(m[W])))and... count_over_time .... The PromQL frontend lowers them to exactSort+Limit, so Pass 1 has no heap candidate for them (unchanged).How
asap-types).SummaryEstimate{TopK}over a keyed build derives[partition columns.., the build's item columns.., topk: Float64]. That is exactly whatOperator::keyed_evaluationemits.[keys.., topk_k]and then, over PromQL rows, the item columns (ranked_as_topk). The fused heap and a top-k's own heap both use it. A SQL top-k keeps only its logical columns, as its exact plan does.whole_expression_inputrefused every closed child. It now refuses only when it needs an encoded label set. TheGlabels are plain item columns.with_confirmed_weights), asSelection::planholds it. Before, a Count-Min heap over counter rates failed with "CMS requires a nonnegative weight contract" even when the counter was declared.__name__from a heap top-k's rows, as it already does for every other temporal summary evaluation.CountMinSketchWithHeap/CountSketchWithHeap(withrescore_heap, fix(executor): DDSketch interpolated quantiles and top-k membership on sketchlib planner-pin/0.4.0-plus #803), unchanged. Nothing was missing from the library.Key code interfaces added / modified
No Rust signature changes. The DAG format changes. A heap top-k estimate node's output schema in the plan file (
PhysicalASAPDAG,InstallablePlan) is now:crates/types/src/ir/operator/asap.rscrates/logical-asap-optimization/src/per_computation_candidates.rsUsage
From
a_forced_heap_topk_sum_plan_compiles_and_answers_within_its_bound(crates/planner/tests/runtime_install.rs), run at the linked commit:Before this PR
With guarantees ignored, every heap top-k candidate was rejected as
ExecutorCannotCompile, because its estimate's declared shape is not the keyed evaluation's. Counter top-k (topk(k, sum by (G) (increase|rate(...)))) got no heap candidate at all throughplan_installable. With the audit's throwaway weight fix, q06 alone selected the heap at 2.243654e-8 and then could not run.After this PR
Heap top-k plans compile and run. Stage 3 picks one when it is the cheapest valid plan. With guarantees ignored, ASAPQuery's q06 now selects the Count-Min heap and passes the converter's check (results below). Together with #815, the heap is a Count-Min heap over a gauge declared non-negative.
Verification
Tests written first:
runtime_install::a_forced_heap_topk_sum_plan_compiles_and_answers_within_its_bound(Count Sketch heap, gauge),..._topk_count_plan_...(both heaps),..._counter_topk_plan_...(increaseandrate, both heaps). They go throughplan_installableand the stand-in runtime on skewed data. Each checks that the forced heap plan compiles (Stage 3's check andcheck_compiles), that the true top 3 is recalled in every partition, that each estimate is within the heap's bound of the exact value (Count-Min:0 ≤ est − exact ≤ (e/w)·L1; Count Sketch:|est − exact| ≤ √(3/w)·L2 ≤ ε·L1), that labels are kept without__name__, and that the query-time reference passes the same checks. Before the fix, the sum test failed with "no valid candidate": the Count Sketch heap was rejected by the compile check and the Count-Min heap by its weights. The counter shape had no heap candidate (Stage 3 checked 1 candidate, the exact one). With only the shape fix, the sum test failed on__name__. The count test was added after those runs and was not run against main.weighted_topk_binding::planner_weighted_topk_binds_at_either_deployment_phaseis un-ignored.topk by (job) (2, sum by (service, job) (rate(m[1m])))with both heaps, at ingestion and at query time, answers the exact top 2 per job. Its plan now has the weights confirmed for a declared counter, as Stage 3 hands it over, and it reads the ranked value fromtopk_2.promql_candidates_composeaccepts a heap top-k root's[keys.., topk_k: Float64, items..]. The Example 1 stage fixture is regenerated (stage_pipeline --exhaustive) and the dag-viewer tests pass. The acceptance doc's Q2 summary shape is updated, and its paragraph on the Example 1 every-candidate compile test is dropped (that ignored test was deleted on main).cargo test --workspace --locked --no-fail-faston the head: 151 test binaries, 1,844 passed, 0 failed, 35 ignored (one fewer ignored than main).Real workloads (seed-862
workload-01.json, the localasapquery_random_workloadconverter rebuilt on this branch with #815 merged in,--no-raw --failure-probability 0.3 --check, 200 series, 3 evaluations):--ignore-accuracy-guarantees--ignore-accuracy-guarantees--ignore-accuracy-guaranteesAs-is, the heap still has no guarantee (no
stream_normsdeclared, and a top-k membership has none), so the exact plan wins there, as expected.Follow-ups, not in this PR:
topk(k, sum by (G) (sum_over_time|count_over_time(...)))has no heap candidate (frontend lowering, above).weighted_topk_bindingtests are about a heap over one row per series (pruned since fix(planner): Stage 3 rejects plans the executor cannot compile; no PromQL top-k heap over its instant vector #805). They stay ignored.