Skip to content

feat(summary_executor): support SketchQuery::TopK via a two-shape Value - #412

Merged
zzylol merged 1 commit into
mainfrom
feat/summary-executor-topk-value
Jul 27, 2026
Merged

zzylol merged 1 commit into
mainfrom
feat/summary-executor-topk-value

Conversation

@zzylol

@zzylol zzylol commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Step 1 of 3 toward retiring the legacy sketch_reducer.rs query path in favor of the plan-based SummaryExecutor merged in #411 — the user wants no duplicate/parallel query-execution implementations, and this was the one functional gap (of two) blocking a full cutover.

SummaryExecutor::Value was Vec<(i64, f64)> — one scalar per point — which can't represent SketchQuery::TopK's answer (K ranked (item, count) pairs per point), so it errored Unsupported. Value is a fully deployment-opaque associated type (no bounds in the upstream ASAPController trait), so this is 100% local to data_plane:

  • New SummaryValue::{Points(Vec<(i64,f64)>), TopK(Vec<(i64, Vec<(String,f64)>)>)}, replacing the old Value type.
  • readout_cumulative/readout_per_window each branch once on matches!(query, SketchQuery::TopK) to decide which shape to produce (merge loop itself is unchanged).
  • New topk_ranked(rs, k) helper: reads SummaryState::topk_items() (already existed), sorts descending by value (load-bearing — the heap's backing array isn't actually ordered despite its own doc comment's claim), caps at k.
  • No new merge/sketch-math anywhere. merge_same_family already reconciles heaps correctly across sids (asap_sketchlib's CountMinSketchWithHeap/CountSketchWithHeap::merge re-queries every candidate key against the merged matrix) — top-k readout just reads the final merged state's heap, reusing the exact same cross-sid merge pipeline every other query already goes through.

Blast radius confirmed via full-crate grep before starting: this executor isn't wired into engine.rs's live ASAPQueryEngine yet, so nothing outside this one file/its own test module is affected.

Test plan

  • cargo test -p data_plane --lib summary_executor — 15 tests (12 existing + 3 new), all passing:
    • single_cms_with_heap_sid_topk_readout_sorted_and_capped — proves sort+cap.
    • two_cms_with_heap_sids_same_group_topk_merges_cross_sid — two sids with disjoint keys, proves the merged answer contains both — the actual cross-sid merge proof.
    • per_window_matrix_topk_produces_per_window_ranked_lists — the one genuinely new code path (readout_per_window's TopK branch), two windows with different top keys each.
    • topk_query_against_non_heap_sketch_is_unsupported (renamed from ..._is_explicitly_unsupported_not_silently_wrong) — still errors for a heap-less family; now documented as a family limitation, not "unimplemented."
  • cargo test -p data_plane --lib — full lib suite: 911 passed, 2 ignored (908 + 3 new).
  • cargo build -p data_plane — clean.
  • cargo clippy -p data_plane --lib --tests — zero warnings in the touched file.
  • rustfmt scoped to the one touched file.

Not in this PR (separate, later steps per the user's own sequencing): wiring SummaryExecutor into engine.rs's live ASAPQueryEngine, the named-key PointCount gap, and deleting sketch_reducer.rs.

🤖 Generated with Claude Code

SummaryExecutor::Value was Vec<(i64, f64)> -- one scalar per point --
which can't represent TopK's "K ranked (item, count) pairs per point"
answer, so it errored Unsupported. Value is a fully deployment-opaque
associated type (no upstream ASAPController trait bounds), so this is
local to data_plane: introduce SummaryValue::{Points, TopK} and branch
readout_cumulative/readout_per_window on the query once each.

No new merge logic needed -- SummaryState::topk_items already decodes
heap items for CmsWithHeap/CountSketchWithHeap, and the existing
merge_same_family pipeline (asap_sketchlib's heap merge) already
reconciles the heap against the merged matrix across sids, so top-k
readout reuses the exact same cross-sid merge every other query goes
through.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol merged commit 8c712cd into main Jul 27, 2026
@zzylol
zzylol deleted the feat/summary-executor-topk-value branch September 12, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant