Skip to content

feat(summary_executor): read out named-key PointCount (count(metric{item=x})) - #414

Merged
zzylol merged 2 commits into
mainfrom
feat/summary-executor-pointcount-readout
Jul 27, 2026
Merged

zzylol merged 2 commits into
mainfrom
feat/summary-executor-pointcount-readout

Conversation

@zzylol

@zzylol zzylol commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase 4 of 5 toward supporting named-key SketchQuery::PointCount (count(cms_metric{item="x"})). Needs SketchQuery::PointCount.value from ASAPController#162 (merged) / #413 (merged) — both landed, so this is now based directly on main.

  • Bumps data_plane's ASAPController rev pin to the merged 64df20d (matching feat(sketch_algebra): realize Frequency (AggIntent::Extension) as a real Cms sketch #413's control_plane pin) to pick up SketchQuery::PointCount.value: Option<String>.
  • New SummaryState::estimate(key) in delta_apply.rs, mirroring topk_items's per-variant dispatch pattern but broader: all four Frequency variants (Cms, CountSketch, CmsWithHeap, CountSketchWithHeap) already carry a keyed estimate over their matrix — no heap needed for a point lookup, unlike TopK.
  • sketch_query_value's PointCount arms now handle both shapes: {key: SampleValue, value: None} (bare total, unchanged behavior) and {key: Named/Qualified, value: Some(v)} (per-item point lookup via SummaryState::estimate), erroring on any other key/value combination rather than guessing.

Scope note: this closes the data_plane-side readout logic only. Threading the actual filter value into whatever constructs SketchQuery::PointCount in the first place (Phase 3 — control_plane's frequency() intent construction) turned out to need substantially more plumbing than expected: recognizing a bare count(metric{item="x"}) PromQL shape and resolving item against a metric's configured item-label requires a new metric→item_label lookup mechanism threaded through parse_query_expr_canonical (18 call sites) and QueryWorkload (31 struct-literal construction sites, no Default impl) — a genuinely larger, separate task, deferred for now. This PR's new tests exercise the readout logic directly with hand-built SketchQuery::PointCount{value: Some(_)} fixtures instead of an end-to-end PromQL string, which is exactly the shape Phase 3 will eventually produce.

Test plan

  • cargo test -p data_plane --lib summary_executor — 17 tests (15 existing + 2 new), all passing:
    • single_cms_sid_named_key_point_estimate — a single sid, point lookup for a specific key, asserts the exact inserted weight.
    • two_cms_sids_same_group_named_key_estimate_merges_cross_sid — two sids contributing to the same key, proves the merged answer is the sum (the cross-sid merge proof, mirroring two_cms_sids_same_group_totals_actually_merge).
  • cargo test -p data_plane --lib — full lib suite: 913 passed, 2 ignored.
  • cargo test -p data_plane --test e2e_controller_plans_and_backend_serves — 2 pre-existing failures (controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk), confirmed identical on the unmodified branch via git stash before/after comparison — not a regression from this change.
  • cargo build --workspace — clean.
  • cargo clippy -p data_plane --lib --tests — 107 warnings, same count as this branch's baseline; verified none fall in the touched files.
  • rustfmt scoped to the two touched files.

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the feat/control-plane-frequency-extension-realize branch from f6f34db to e697463 Compare July 27, 2026 12:36
zzylol and others added 2 commits July 27, 2026 06:39
…tem=x}))

Bumps the ASAPController rev pin to e4cea66 (feat/extension-realization-hook,
ASAPController#162, not yet merged) to pick up
SketchQuery::PointCount.value: Option<String>.

Adds SummaryState::estimate(key) in delta_apply.rs, mirroring topk_items's
per-variant dispatch but broader: all four Frequency variants (Cms,
CountSketch, CmsWithHeap, CountSketchWithHeap) already carry a keyed
estimate over their matrix -- no heap needed, unlike TopK.

summary_executor.rs's sketch_query_value now handles PointCount{key:
SampleValue, value: None} (bare total, unchanged) and PointCount{key:
Named/Qualified, value: Some(v)} (per-item point lookup via
SummaryState::estimate), erroring on any other key/value combination
instead of guessing.

This closes the data_plane side of the named-key PointCount gap
(Phase 4 of 5). Threading the actual filter value into the query
pipeline that constructs SketchQuery::PointCount (Phase 3) turned out to
require substantially more plumbing than expected -- a new
metric-to-item_label lookup mechanism through a previously pure-syntax
PromQL parsing pipeline -- and is deferred as a separate, larger task;
this PR's new tests exercise the readout logic directly with hand-built
SketchQuery::PointCount{value: Some(_)} fixtures rather than an
end-to-end PromQL string.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
64df20d is ASAPController main's actual tip now that #162 merged --
replaces the temporary pre-merge pin at e4cea66, matching control_plane's
own pin from #413.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol changed the base branch from feat/control-plane-frequency-extension-realize to main July 27, 2026 12:41
@zzylol
zzylol force-pushed the feat/summary-executor-pointcount-readout branch from d7a78a8 to dcc0af6 Compare July 27, 2026 12:41
@zzylol
zzylol merged commit fdf4f9b into main Jul 27, 2026
@zzylol
zzylol deleted the feat/summary-executor-pointcount-readout 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