feat(sketch_algebra): route Capability::is_satisfied_by through SummaryFamilyMatcher (Step 4, scoped down) - #402
Merged
Conversation
zzylol
force-pushed
the
feat/capability-uses-summary-family-matcher
branch
2 times, most recently
from
July 21, 2026 16:04
a83c0dc to
2ab5a13
Compare
…ryFamilyMatcher Step 4 of the sketch-identity unification (see scratchpad/artifacts/enum-unification-plan.md), scoped down after investigation found the "delete Capability/SketchKindHandle entirely" premise underestimated the blast radius: SketchInstanceMetadata.capability: Option<Capability> is a pervasive in-memory dispatch field across data_plane's query engine, otel ingest, http driver, reconcile lifecycle, benches, examples, and tests -- roughly 500 combined references across ~20 files (asap_tier_analysis.rs alone has 72, otel.rs 62, engine.rs 57, sketch_reducer.rs 52), far beyond what the plan's file list anticipated and well past what's safely verifiable in one PR. Per the plan's own risk-scoping guidance (exactly like Stage 4 of the earlier re-layering work got scoped down from 4 items to 3), this PR lands the real matching-logic consolidation without deleting the types themselves. What changed: - Capability::is_satisfied_by's four sketch-family arms (QuantileApprox, CardinalityApprox, FrequencyEstimate, FrequencyTopk) now delegate to a new sketch_algebra::matcher::sketch_family_satisfied free function -- the same family-compatibility rule SummaryFamilyMatcher already applies to asap_plan::Implementation values -- instead of the hand-rolled handles_compatible/handles_compatible_for_topk/ is_heap_bearing/is_frequency_family helpers (deleted). One rule table, not two. - SketchKindHandle::Any (never indexed against a concrete sketch instance, only ever appears on the required side) resolves to a concrete per-family stand-in before hitting the family check, since SummaryKind has no wildcard concept -- family-matching subsumes it. The FrequencyTopk stand-in is specifically the heap-bearing CmsWithHeap, not bare Cms: a bare stand-in would let a heap-less available sketch wrongly satisfy a top-k requirement, since bare Cms/CountSketch and CmsWithHeap/CountSketchWithHeap are members of the SAME family (related by the asymmetric "heap satisfies bare" rule, not equal). - Net effect is an intentional broadening for concrete (non-Any) required handles: family membership now decides satisfaction regardless of whether the requirement spelled out `Any` or a concrete kind (matching the plan's §5 table, which isn't conditioned on that distinction). Verified harmless: capability_for -- the only production constructor of a required Capability -- never emits a concrete handle, only Any, for every sketch-family variant. Two defensive/theoretical unit tests asserting the old exact-match behavior were updated in place with comments explaining the change. - Added exact_summary_kind_for(AggregationType) -> Option<SummaryKind>, a pure, tested mapping from data-plane's AggregationType (which conflates identity + keyed/unkeyed) onto SummaryKind's exact- accumulator identity axis, for Step 5 or merge-time reconciliation. Deliberately NOT wired into Capability::ExactAgg: doing so would silently drop the keyed-vs-unkeyed asymmetric rule (multi_pop_satisfies_single) that ExactAgg's own is_satisfied_by arm depends on, since SummaryKind alone can't distinguish a multi-population policy once Sum/MultipleSum collapse onto one variant. Fixing that properly needs a grouping sibling field, which is Step 5's territory (aggregation_config.rs) and out of scope here. Investigated and confirmed (not taken on faith): - Capability is never persisted. metadata.rs's SidMetaRecord::capability() derives it fresh from agg_kind on every load; the only durable artifact is the 8-string sketch_kind_to_str/from_str vocabulary in the same file, which this PR does not touch (SketchKindHandle is unchanged) -- sid_metadata.json round-tripping is unaffected. - timeline.rs's sketch_kind_byte fixed-mapping feeds AggSignatureGroup.signature_id, computed fresh at query time from live in-memory metadata (xxh64 over a canonical encoding), never read back from disk -- confirmed not a persistence concern, also untouched here since SketchKindHandle is unchanged. Deferred (not done in this PR): full deletion of Capability/ SketchKindHandle and re-pointing data_plane's re-export at asap_plan::Implementation directly. That remains real Step 4 scope for a future, separately-reviewed PR once each of the ~20 data_plane call sites (and the SketchInstanceMetadata.capability field itself) has been individually migrated and verified -- not safe to do speculatively here. Overlap note for the concurrent Step 5 branch (splitting data_plane's AggregationType into AccumulatorSpec): this PR does not touch aggregation_config.rs or accumulator_factory.rs. The only AggregationType-touching lines are the new exact_summary_kind_for mapping function and its tests in capability.rs (both pure additions, no existing call site changed) -- reconciliation should be low-friction. - cargo build -p control_plane -p data_plane -p asap_types: clean - cargo test -p control_plane: 827 passed (822 baseline + 5 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default) - cargo test -p data_plane --lib: 881 passed, 0 failed (matches baseline exactly) Rebased onto main post-PR #395 (phase2/query-expr-relational-merge, merged after this branch was opened). #395 heavily rewrote this same file: capability_for now delegates to asap_plan::boundary::implementation_for via implementation_to_capability, and a follow-up fix on main added sum_satisfies_increase so a Sum-registered sid can answer a required Increase/Rate capability. The rebase conflict was confined to the #[cfg(test)] module: both branches appended new tests directly after exact_agg_covers_each_canonical_agg_type -- main's is_satisfied_by_sum_family_answers_required_increase / is_satisfied_by_sum_family_does_not_answer_required_multi_increase_from_single_sum / is_satisfied_by_increase_does_not_answer_required_sum (covering sum_satisfies_increase) and this branch's exact_summary_kind_for_maps_bare_identity_variants / exact_summary_kind_for_maps_keyed_siblings_onto_the_same_kind / exact_summary_kind_for_rejects_sketch_shaped_variants (covering exact_summary_kind_for). Resolution kept both blocks, each with its own closing brace (git's diff had folded them under one shared trailing `}` belonging to whichever side's last test happened to end there). Outside the tests module the rebase auto-merged cleanly: capability_for's delegation to implementation_for, is_satisfied_by's ExactAgg arm (req == have || multi_pop_satisfies_single || sum_satisfies_increase), and this PR's own sketch_kinds_compatible -> sketch_family_satisfied routing for the four sketch-family arms all coexist as intended, with no further edits needed. matcher.rs's sketch_family_satisfied free function (this PR's own commit) applied cleanly with no conflict. Post-rebase: cargo build --workspace clean. cargo test -p asap_types: 36 passed. cargo test -p control_plane --lib: 774 passed, 1 failed (optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default, confirmed pre-existing and failing on main itself, unrelated to this change). cargo test -p data_plane --lib: 881 passed, 0 failed, 2 ignored. The control_plane baseline moved from 822 to a lower number purely because main's history advanced past this branch's original fork point (unrelated intervening merges); a net-tests diff against origin/main confirms zero tests were dropped by this rebase (+5 net new, 0 removed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
feat/capability-uses-summary-family-matcher
branch
from
July 21, 2026 20:55
2ab5a13 to
db993fd
Compare
This was referenced Sep 11, 2026
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
1 participant
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
Step 4 of the sketch-identity unification (
scratchpad/artifacts/enum-unification-plan.md), scoped down after investigation. The original plan was "deleteCapability/SketchKindHandleentirely, re-pointdata_plane's re-export atasap_plan::Implementation." Grepping the real blast radius foundSketchInstanceMetadata.capability: Option<Capability>is a pervasive in-memory dispatch field acrossdata_plane's query engine, otel ingest, http driver, reconcile lifecycle, benches, examples, and tests — roughly 500 combined references across ~20 files (asap_tier_analysis.rsalone has 72 hits,otel.rs62,engine.rs57,sketch_reducer.rs52). That's far beyond what the plan's file list anticipated and past what's safely verifiable in one PR — so, per the plan's own risk-scoping guidance (the same call Stage 4 of the earliersketch_algebrare-layering made, scoping down from 4 items to 3), this PR lands the real matching-logic consolidation without deleting the types.What changed
Capability::is_satisfied_by's four sketch-family arms (QuantileApprox,CardinalityApprox,FrequencyEstimate,FrequencyTopk) now delegate to a newsketch_algebra::matcher::sketch_family_satisfiedfree function — the same family-compatibility ruleSummaryFamilyMatcheralready applies toasap_plan::Implementationvalues — instead of the hand-rolledhandles_compatible/handles_compatible_for_topk/is_heap_bearing/is_frequency_familyhelpers (deleted). One rule table, not two.SketchKindHandle::Anyresolves to a concrete per-family stand-in before hitting the family check, sinceSummaryKindhas no wildcard concept — family-matching subsumes it. TheFrequencyTopkstand-in is specifically heap-bearingCmsWithHeap, not bareCms— a bare stand-in would let a heap-less available sketch wrongly satisfy a top-k requirement.exact_summary_kind_for(AggregationType) -> Option<SummaryKind>, a pure, tested mapping ontoSummaryKind's exact-accumulator identity axis, for Step 5 or merge-time reconciliation to consume. Not wired intoCapability::ExactAgg— doing so would silently drop the keyed-vs-unkeyed asymmetric rule (multi_pop_satisfies_single) that arm depends on.Investigated and confirmed
Capabilityis never persisted.metadata.rs'sSidMetaRecord::capability()derives it fresh fromagg_kindon every load; the only durable artifact is the 8-stringsketch_kind_to_str/from_strvocabulary, untouched here.timeline.rs'ssketch_kind_bytefeeds a query-time-only hash (AggSignatureGroup.signature_id), never read back from disk — confirmed not a persistence concern.Intentional behavior change (verified harmless)
Family membership now decides satisfaction for concrete (non-
Any) required handles too, not justAny.capability_for— the only production constructor of a requiredCapability— never emits a concrete handle for any sketch-family variant, onlyAny, so this path is exercised defensively/in-tests only. Two such tests were updated in place with comments explaining the change.Deferred (real Step 4 scope, future PR)
Full deletion of
Capability/SketchKindHandleand re-pointingdata_plane's re-export atasap_plan::Implementationdirectly — needs each of the ~20 data_plane call sites individually migrated and verified, not safe to do speculatively here.Overlap with concurrent Step 5 (
AggregationType→AccumulatorSpecsplit)Does not touch
aggregation_config.rsoraccumulator_factory.rs. The onlyAggregationType-touching lines are the newexact_summary_kind_formapping function and its tests (pure additions, no existing call site changed) — reconciliation should be low-friction.Test plan
cargo build -p control_plane -p data_plane -p asap_types: cleancargo test -p control_plane: 827 passed (822 baseline + 5 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default)cargo test -p data_plane --lib: 881 passed, 0 failed (matches baseline exactly)cargo fmt -p control_plane; reverted incidental reformatting outside this diff🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com