feat(sketch_algebra): realize Frequency (AggIntent::Extension) as a real Cms sketch - #413
Merged
Merged
Conversation
6 tasks
…eal Cms sketch
Bumps the ASAPController rev pin to feat/extension-realization-hook
(ASAPController#162, not yet merged) to pick up
CostModel::{realize_extension, readout_extension} (ASAPController#150).
ControlPlaneCostModel now implements both for ext_kind == "frequency":
realize_extension parses the accuracy out of payload and returns
Implementation::Sketch{kind: Cms, params: Cms{width,depth}} -- matching
capability_matching::pick_family's own Frequency -> Cms mapping (NOT the
older sketch_catalog::sketch_type_for_op's SketchType::CountSketch,
which is a genuinely different sketch algorithm under a similar name and
predates/disagrees with capability_matching -- kept the newer, tested
mapping). readout_extension builds SketchQuery::PointCount, falling back
to {key: SampleValue, value: None} until the filter value is threaded
through payload (a separate, later change) -- correct as the bare
bucket total in the meantime.
ForcedFamilyCostModel (used by optimizer::rules::bind_workload_typed)
delegates both new methods to its inner ControlPlaneCostModel, so the
`endpoint_request_freq` contract row -- previously a documented, accepted
regression declining to Logical pending #150 -- now commits like every
other row. Flipped the tests that pinned the old decline behavior to
assert the new Cms binding instead.
Also updates emit::stage_config's SketchQuery::PointCount wire JSON for
the new `value` field (ASAPController#162).
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. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
feat/control-plane-frequency-extension-realize
branch
from
July 27, 2026 12:36
f6f34db to
e697463
Compare
…ding Caught in review: the section header above frequency_extension_binds_cms still described Frequency as declining to bind (pre-#150 behavior) and pointed at the old, renamed test name. Corrected to describe current behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Phase 2 of 5 toward supporting named-key
SketchQuery::PointCount(count(cms_metric{item="x"})) — see the earlier phase (data_plane'sSketchQuery::TopKsupport, #412) and the upstream prep (ASAPController#162, not yet merged).Blocked on ASAPController#162 merging. This PR temporarily bumps
control_plane'sasap-ir/asap-l2/asap-sketch/asap-planrev pins toe4cea66(thefeat/extension-realization-hookbranch commit, ASAPController#162) so this can be built/tested now. Re-pin to the actual merged commit once #162 lands — same pattern as thefc09c3a→d4c1756bump before ASAPController#161 merged.AggIntent::Extension{ext_kind: "frequency"}(control_plane's point-frequency-via-CMS intent,count(*) WHERE key = k) has always mapped toImplementation::PassThrough— a documented, accepted regression (typed_binding_endpoint_request_freq_declines_pending_upstream_extension_support) since Step B of the plan-shaped-serving migration adoptedasap_plan::bind::implement_tree_in_withwithout a local Frequency-binding workaround. ASAPController#150/#162 added theCostModel::{realize_extension, readout_extension}hooks needed to close this; this PR implements both onControlPlaneCostModel:realize_extension("frequency", payload)parses the accuracy out ofpayloadand returnsImplementation::Sketch{kind: SummaryKind::Cms, params: Cms{width, depth}}.readout_extensionbuildsSketchQuery::PointCount, falling back to{key: SampleValue, value: None}(the bare bucket total) until the actual filter value is threaded throughpayload— a separate, later change (Phase 3).ForcedFamilyCostModel(used byoptimizer::rules::bind_workload_typed) delegates both new methods to its innerControlPlaneCostModel, so theendpoint_request_freqcontract row now commits instead of declining.A genuine discrepancy found and resolved along the way: the legacy
sketch_catalog::sketch_type_for_op'sas_frequencyarm maps Frequency toSketchType::CountSketch, while the newer, actively-testedcapability_matching::pick_family/is_valid_pairmaps it toSummaryKind::Cms— these are genuinely different sketch algorithms (SketchTypehas separateCountSketch/CountMinSketchvariants), not a naming coincidence. Went withcapability_matching's mapping since it's the tested, current source of truth for the typed binding path this change lives in; the legacysketch_catalog-driven planner (a separate, still-coexisting code path, untouched by this PR) keeps its ownCountSketchchoice — not something this PR should or does reconcile.Also updates
emit::stage_config'sSketchQuery::PointCountwire-JSON builder for the newvaluefield (ASAPController#162).Test plan
cargo test -p control_plane --lib— 765 passed, 1 failed (the same single pre-existing, unrelated failure —invalid_sketch_type_override_falls_back_to_default— present on unmodified main too).typed_binding_endpoint_request_freq_binds_cms(renamed),sketch_algebra::tests::frequency_extension_binds_cms(renamed), plus theCountMinSketch-override case inoverride_pins_nonquantile_family_over_misclassified_queryand the two routing-table fixtures inemit::mod.rs.cargo test --workspace— same single pre-existing failure, nothing else.cargo build --workspace— clean.cargo clippy -p control_plane --lib --tests— verified every warning falls outside this diff's touched line ranges (all pre-existing).rustfmtscoped to touched files only.🤖 Generated with Claude Code