feat(sketch_algebra): implement Matcher for asap_plan::Implementation - #399
Merged
Merged
Conversation
Step 1 of the sketch-identity unification (see scratchpad/artifacts/enum-unification-plan.md). Bumps the ASAPController pin (asap-ir, plus newly-added asap-plan/asap-sketch deps) to 283b6ab (merge of PR #141) to pick up Matcher/Implementation and SummaryKind/SummaryParams. SummaryFamilyMatcher restores, verbatim, the family-compatibility logic that briefly lived as a concrete Implementation::is_satisfied_by method in asap-plan (PR #140) before Stage 1 of this migration converted it into a trait with no default implementation (PR #141) -- pulled from ASAPController's git history (commit da8fa75, the last one with the corrected per-kind params_for test helper) rather than reconstructed from memory, so the 10 ported tests are a faithful restoration, not a reinterpretation. Scope note: this deliberately does NOT implement the single-vs-multi-population re-aggregation rule (e.g. a keyed Sum accumulator serving an unkeyed Sum query) -- Implementation carries no grouping information, so a two-Implementation Matcher can't correctly answer that question. That logic already exists as production code in asap_types::capability_matching::find_compatible_aggregation, which checks type compatibility and grouping-label subset-compatibility side by side. The two checks compose at the caller. This is additive -- no existing enum (Capability, SketchKindHandle, sketch_algebra::SketchKind) is touched or deleted yet. That's later steps in the same plan. - cargo build --workspace: clean - cargo test -p control_plane: 834 passed (824 + 10 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
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
Step 2 of the sketch-identity unification plan (design doc — not committed to the repo, see conversation for the published artifact). Step 1 (audit whether ASAPController needs serde on
SummaryKind/SummaryParams) concluded no — every real emit path incontrol_planebuilds JSON/YAML by hand (build_backend_aggregation_jsonetc.), never viaserde_json::to_valueon a typed struct; the existingSerialize/Deserializederives onsketch_algebra::SketchKind/SketchParamsonly back#[cfg(test)]round-trip tests. No ASAPController PR needed — folded into this repo's own work instead.This step:
283b6ab(merge of PR refactor(legacy_expr): step γ4 — TopK bridge (heavy-hitter vs Sort+Limit split) #141, "convertImplementation::is_satisfied_byinto aMatchertrait") and addsasap-plan/asap-sketchas newcontrol_planedependencies (previously onlyasap-irwas pinned).SummaryFamilyMatcher: Matcher— restores, verbatim, the family-compatibility logic that briefly lived as a concreteImplementation::is_satisfied_bymethod inasap-plan(PR refactor(legacy_expr): step γ1 — Aggregate bridge module + output_schema helper #140) before it became a trait with no default impl (PR refactor(legacy_expr): step γ4 — TopK bridge (heavy-hitter vs Sort+Limit split) #141). Pulled directly from ASAPController's git history (commitda8fa75) rather than reconstructed from memory, so all 10 ported tests are a faithful restoration.Scope note: this deliberately does not implement the single-vs-multi-population re-aggregation rule (e.g. a keyed
Sumaccumulator serving an unkeyedSumquery) —Implementationcarries no grouping information at all, so a two-ImplementationMatchercan't correctly answer that question. That logic already exists as production code inasap_types::capability_matching::find_compatible_aggregation, which checks type compatibility and grouping-label subset-compatibility side by side; the two checks compose at the caller, not inside a singleMatcher::is_satisfied_by.This PR is purely additive — no existing enum (
Capability,SketchKindHandle,sketch_algebra::SketchKind) is touched or deleted. That's later steps in the same plan.Test plan
cargo build --workspace— cleancargo test -p control_plane— 834 passed (824 + 10 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default)🤖 Generated with Claude Code