feat(plan): pluggable AggIntent::Extension realization + readout hook (closes #150) - #162
Merged
Merged
Conversation
Closes the gap tracked in #150: boundary::implementation_for_with mapped every AggIntent::Extension unconditionally to Implementation::PassThrough, with no way for a deployment to opt in to its own realization for a shape core doesn't know (e.g. control_plane's Extension{"frequency"} point-frequency intent). Adds two CostModel methods, both defaulting to today's behavior so no existing deployment is affected: - realize_extension(ext_kind, payload) -> Implementation, consulted by implementation_for_with's Extension arm instead of hardcoding PassThrough. - readout_extension(ext_kind, payload, col) -> SketchQuery, consulted by bind::readout's new Extension arm. This half wasn't in #150's original ask, but is required for the feature to work end-to-end: readout() previously had `other => unreachable!()` for anything besides Quantile/Cardinality/TopK/Count, so a CostModel that realizes an Extension as Sketch without this would panic the first time it's read out. Also adds SketchQuery::PointCount.value: Option<String>, needed by readout_extension to carry a per-item filter value (e.g. count(cms_metric{item="checkout"}) -> PointCount{key: Named("item"), value: Some("checkout")}) -- SketchQuery is a closed, exhaustively-matched enum with no room to grow this otherwise, and no SummaryExecutor implementation has any way to resolve a filter value on its own (readout gets no tree access). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original 37-commit history hit real conflicts with the intervening Reduction/L4 grouping-model work -- squashed to one commit here since the PR merges as a squash anyway, so the intermediate history wasn't worth preserving through a 37-way conflict resolution. `docs/design.md` was a 1500+ line document mixing layer design, migration planning, and crate-by-crate speculation, most of it already stale relative to the shipped code. This does the same move `l4node-execution- model.md` already made for L4 across the rest of the pipeline: each layer gets its own doc, `design.md` becomes a thin index into them, and every doc is cross-checked against current code rather than copied from old drafts. - New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`, `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed, renamed to `l4-summary-bound-ir.md` to match the shipped `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older "sketch" framing). `l1-query-language.md` also absorbs the old standalone `promql-lowering.md` as a section. - `design.md` cut to a slim index: one paragraph per L1-L5 linking to its doc, plus a trimmed Glossary. - Deleted `docs/migration-plan.md` and `docs/intent-algebra- reconciliation.md` -- both were migration-era planning artifacts, the latter already self-annotated as a superseded historical snapshot. - `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed `l4-summary-bound-ir.md`. Known gap carried over from the original PR (not introduced by this rebase): README.md still links to `docs/migration-plan.md` and to `docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of which exist after this change -- worth a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original 37-commit history hit real conflicts with the intervening Reduction/L4 grouping-model work -- squashed to one commit here since the PR merges as a squash anyway, so the intermediate history wasn't worth preserving through a 37-way conflict resolution. `docs/design.md` was a 1500+ line document mixing layer design, migration planning, and crate-by-crate speculation, most of it already stale relative to the shipped code. This does the same move `l4node-execution- model.md` already made for L4 across the rest of the pipeline: each layer gets its own doc, `design.md` becomes a thin index into them, and every doc is cross-checked against current code rather than copied from old drafts. - New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`, `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed, renamed to `l4-summary-bound-ir.md` to match the shipped `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older "sketch" framing). `l1-query-language.md` also absorbs the old standalone `promql-lowering.md` as a section. - `design.md` cut to a slim index: one paragraph per L1-L5 linking to its doc, plus a trimmed Glossary. - Deleted `docs/migration-plan.md` and `docs/intent-algebra- reconciliation.md` -- both were migration-era planning artifacts, the latter already self-annotated as a superseded historical snapshot. - `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed `l4-summary-bound-ir.md`. Known gap carried over from the original PR (not introduced by this rebase): README.md still links to `docs/migration-plan.md` and to `docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of which exist after this change -- worth a follow-up. 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
Closes #150:
boundary::implementation_for_withmapped everyAggIntent::Extensionunconditionally toImplementation::PassThrough, with no way for a deployment to opt in to its own realization for a shape core doesn't know (e.g.control_plane'sExtension{"frequency"}point-frequency intent —count(*) WHERE key = k).Two new
CostModelmethods, both with safe defaults so every existing deployment is unaffected:realize_extension(ext_kind, payload) -> Implementation— consulted byimplementation_for_with'sExtensionarm instead of hardcodingPassThrough. Default:PassThrough(today's behavior).readout_extension(ext_kind, payload, col) -> SketchQuery— consulted bybind::readout's newExtensionarm. This half wasn't in boundary::implementation_for maps AggIntent::Extension to PassThrough unconditionally — no deployment-specific realization hook #150's literal ask, but turned out to be required for the feature to work end-to-end:readout()previously hadother => unreachable!(...)for anything besidesQuantile/Cardinality/TopK/Count— aCostModelthat realizes anExtensionasSketchviarealize_extensionalone would panic the first timereadout()actually fires for it. Default panics loudly with a clear message pointing at the missing override, rather than silently misinterpretingpayload.Also adds
SketchQuery::PointCount.value: Option<String>— needed byreadout_extensionto carry a per-item filter value (e.g.count(cms_metric{item="checkout"})→PointCount{key: Named("item"), value: Some("checkout")}).SketchQueryis a closed, exhaustively-matched enum with no room to grow this otherwise, and noSummaryExecutorimplementation has any way to resolve a filter value on its own —readout()'s trait signature has no tree/QueryExpraccess at all, only the already-decodedstate+query.This is prep work for ASAPQuery-backend's
data_planeto close its ownSketchQuery::PointCountnamed-key gap (companion to theSketchQuery::TopKgap closed in ASAPQuery-backend#412) —control_planewill implement both new methods for itsExtension{"frequency"}intent in a follow-up PR once this merges.Test plan
cargo test -p asap-plan -p asap-sketch— 39 tests, all passing, including 2 new:extension_intent_stays_logical_by_default(regression check — anExtensionintent with noCostModeloverride still staysLogical, unchanged) andextension_intent_binds_via_custom_cost_model(a customCostModeloverriding both hooks binds a realSummaryAgg/SummaryEstimatewith the expectedSketchKind/SketchQuery::PointCount{key, value}instead ofLogical/a panic).cargo test --workspace— full workspace, 400+ tests across all crates, all passing.cargo build --workspace— clean.cargo clippy --workspace --all-targets— zero warnings.rustfmtscoped to the touched files.🤖 Generated with Claude Code