Conversation
|
Found while auditing other open PRs for the same tree-vs-DAG class of bug as PR #243: Fixed: New regression test (
|
) Adds crates/asap-aware-mapping/src/applicability.rs: an ApplicabilityRule trait (same extension-point shape as CostModel and Matcher elsewhere in this crate), find_applicable_optimizations(_with) entry points, and two real rules wrapping existing decision logic: - SketchApplicabilityRule wraps boundary::implementation_for_with, reporting "sketch-approximation is applicable" for every bindable Aggregate node the sketch-vs-exact boundary already realizes as a sketch. - SharedSubexpressionRule wraps asap_types::pre_asap::cse::share_common_subtrees, reporting "shared-subexpression reuse is applicable" wherever two or more workload locations end up referencing the same interned Rc<QueryExpr> after CSE (cross-statistic / cross-metric / cross-subpopulation reuse, across roots or within one query). Neither rule re-derives a decision that already exists elsewhere; both just reframe an existing pass as a finding. Every other catalog primitive (wavelets, sampling, deep generative compression, roll-ups, approximation frameworks for windows, function decomposition, continuous distributed monitoring, delta encoding) has no real implementation in this codebase yet, so none get a rule — OptimizationKind is #[non_exhaustive] and the module doc's table names where each would hook in once its primitive exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…des like SharedSubexpressionRule does collect_sketch_findings (SketchApplicabilityRule) walked the workload as a tree, with no Rc-identity tracking - unlike SharedSubexpressionRule's register_site, which explicitly dedupes by Rc::as_ptr. A bindable Aggregate reachable via two paths after share_common_subtrees (two different roots, or two branches of one root, e.g. median(x) / median(x)) was reported as two separate "sketch-approximation is applicable" findings, even though it's bound exactly once - one CostModel::cse_share_decision per shared Rc, per bind::implement_workload_with, not one per path that reaches it. Overstated the number of independent opportunities. for_each_operator_child now passes an Option<usize> Rc pointer alongside each child (None only for Concat's by-value branches, which have no Rc identity of their own) so collect_sketch_findings can thread a `visited` HashSet across the whole evaluate() call - shared across all roots, not reset per root - and skip re-descending into an already-visited pointer, mirroring register_site's exact dedup rationale. New test: a_shared_sketchable_aggregate_is_reported_only_once, the same median(x)/median(x) shape pre_asap::cse's own single_query_shares_its_own_repeated_subtree test uses, pinning that a single shared Aggregate produces exactly one finding. Found while auditing other open PRs for the same tree-vs-DAG issue fixed in PR #243 (default_cse_recompute_cost's naive serde_json-length proxy over-counting internally-shared subtrees). Verified: cargo build --workspace --all-targets, cargo test --workspace (444 passed, 0 failed), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rBridge rename #246 (merged after this branch was forked) renamed QueryExpr::PromqlScalar(f64) to PromqlScalarBridge(Rc<QueryExpr>). walk_rc_children and for_each_operator_child still referenced the removed variant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dac6f10 to
9745842
Compare
|
Superseded by the ReplacementStrategy/Cascades-Volcano search framework built out for issue #33: #259 (#251), #260 (#253), #261 (#256), #262 (#254), #263 (#252), and #264 (#257) — #264 specifically rebuilds this PR's |
…, open issue #33 sub-issues (#258) * docs(asap-aware-mapping): flesh out ReplacementStrategy/search design, link issue #33 sub-issues docs/asap_aware_mapping.md already stubbed TargetSubDAG/ReplacementSubDAG/ ReplacementStrategy/CostModel and a Volcano/Cascades-style search pseudocode as "not yet implemented" (since #206/#211). This turns that stub into an actionable design and ties each piece to a tracked sub-issue of #33: - #251: generalize today's single-pick decisions (boundary::implementation_for, cse::share_common_subtrees) into real ReplacementStrategy impls that report every valid candidate instead of collapsing to one. - #252: the Cascades/Volcano-style search engine itself — MEMO-based candidate plan space (not a flat plan list), deduped via pre_asap::cse's existing structural-hash/InternTable machinery, sorted by CostModel. Explicitly reconciled against docs/cse-cost-model-decision.md (#237), which scoped a narrower binary share/don't-share decision away from full search infrastructure — #252 is the multi-axis case that decision itself flagged as the reason a real engine would eventually be needed. - #253: semantic-equivalent rewriting (avg -> sum/count) as a ReplacementStrategy, no longer needing its own bespoke before/after-CSE heuristic once a real search exists to let both forms compete on cost directly. - #254: group-by-lattice roll-up reuse (AHA vs. independent per-subpopulation treatment) as a ReplacementStrategy, gated on agg_is_mergeable and Schema::unique_keys the same way CSE's own legality gate is. - #256: GroupingStrategy::{PerSubpopulationInstance, SharedMultiSubpopulation} (Hydra) as a new axis orthogonal to SketchKind/SamplingKind/..., named for sharing across a query's own subpopulations (not multi-tenant deployment isolation). - #257: rebuild applicability.rs (#247) as a view over the search's candidate space instead of a parallel tree-walking system. Also resolves the doc's stale "(TODO: is this implemented?)" on parameter sizing — boundary::implementation_for's bind_summary_with already sizes every candidate via CostModel::size_params. No code change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * changes for readability * docs(asap-aware-mapping): plain-English rewrite per Milind's PR #258/#259 comments - MEMO-sharing bullet: spell out why flat plan lists blow up and how Rc-sharing avoids it, without leaning on jargon. - Drop the docs/cse-cost-model-decision.md (#237) history/scope-change framing; keep just the forward-looking design rationale. - Applicability-reporting paragraph: explain MEMO groups concretely instead of the abstract 'candidate-plan space' framing. * Revise ASAP-aware mapping documentation for clarity Updated the documentation for ASAP-aware mapping to improve clarity and structure. Enhanced sections on key concepts, goals, and design principles, and corrected formatting issues. * Enhance glossary and definitions in ASAP-aware mapping Added glossary terms and clarified definitions related to ASAP-aware mapping, including distinctions between alternatives and candidate plans. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Milind Srivastava <milindsrivastava1997@gmail.com>
…nSpace (#257, part of #33) Absorbs PR #264 (feat/applicability-view-257) into this branch, re-derived against the current tip rather than merged/rebased/cherry-picked (#264's base predates the SketchFamilyStrategy->SketchAlgorithmStrategy rename, the search.rs->replacement.rs merge, and the "site"->"target"/TargetSubDAG rename). Replaces the deleted PR #247 rule-based traversal with a pure view over replacement::PlanSpace: a TargetSubDAG is applicability-worthy exactly when its candidate list contains something beyond the trivial, no-op realization. - OptimizationKind::SketchApproximation — the candidate list contains a Replacement::Summary that realizes SummaryFamilyType::Sketch(..). - OptimizationKind::CommonSubexpressionReuse — consumer_count >= 2 and the candidate list contains SharedSubtreeStrategy's "build once and share" candidate. Each finding's `reason` is the matching candidate's own ReplacementSubDAG::rationale verbatim — no new prose. No new applicability-specific extension-point trait: a new finding needs a new impl ReplacementStrategy wired into default_strategies/default_strategies_with regardless, since that's the only way a candidate reaches PlanSpace at all. Kept as its own module (crates/asap-aware-mapping/src/applicability.rs, `pub mod applicability;` in lib.rs) rather than flattened into replacement.rs: unlike implementation.rs/search.rs, this is a reporting/presentation transformation (internal PlanSpace -> external ApplicabilityFinding shape for a downstream DAG-visualization consumer), a genuinely different kind of concern from generating or ranking candidates. Also fixes now-stale cross-references in replacement.rs that pointed at the deleted PR #247 ApplicabilityRule/SharedSubexpressionRule/register_site traversal, and a stale reference in the developer guide (section 4.2) to the same deleted trait. Adds an "Applicability reporting" section (§19) and an extension-map row to docs/developer_docs/ASAP-aware-mapping-developer-guide.md; docs/design_docs/asap_aware_mapping.md already carries a conceptual "Applicability Reporting" section and, matching this session's convention of keeping that document implementation-agnostic, is left as-is. cargo build --workspace --all-targets, cargo test --workspace, cargo fmt --all -- --check, and cargo clippy --workspace --all-targets --all-features -- -D warnings all pass clean. PR #264 (feat/applicability-view-257) and PR #263 (feat/cascades-search-252) are both now superseded by this branch; flagged for the user to close. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Part of #33.
What this adds
crates/asap-aware-mapping/src/applicability.rs: a general, extensible applicability-rule framework — given a workload's pre-ASAP query roots, determine which known optimizations apply, where, and why.Shape (mirrors this crate's existing extension points —
CostModel,Matcher):ApplicabilityRule—fn optimization(&self) -> OptimizationKind+fn evaluate(&self, roots: &[(String, Rc<QueryExpr>)]) -> Vec<ApplicabilityFinding>. A new optimization gets a newimpl ApplicabilityRule, no restructuring required.ApplicabilityFinding { optimization: OptimizationKind, location: String, reason: String }— one positive result: the optimization, a human-readable location in the workload, and a human-readable reason.OptimizationKindis#[non_exhaustive]— only optimizations with a real rule behind them get a variant.find_applicable_optimizations(roots)— the entry point: runsshare_common_subtreesonce (so every rule, including non-CSE ones, sees the same already-deduplicated treeimplement_workloadwould), then evaluatesdefault_rules().find_applicable_optimizations_with(roots, rules)swaps in a custom rule set (e.g. a deployment-specific rule, orSketchApplicabilityRule::newwith a customCostModel).The two real rules, and why these two
Both wrap an existing, already-correct decision procedure as a finding — neither re-derives anything:
SketchApplicabilityRulewrapsboundary::implementation_for_with(the sketch-vs-exact boundary, issue L4: implement the L3→L4 binding (AggIntent→SummaryKind) and the sketch-vs-exact boundary decision #98). For every bindableAggregatenode (same single-intent, no-HAVINGshapebind::implement_treeitself requires), if the boundary decision comes backImplementation::Sketch, that's a "sketch-approximation is applicable" finding. This is literally the codebase's one existing, fully-implemented instance of "optimization applicability" — just reframed as a finding instead of a bind-time side effect.SharedSubexpressionRulewrapsasap_types::pre_asap::cse::share_common_subtrees(issue New feature: add common sub expr elimination #212/CSE detection should run in pre-ASAP IR; post-ASAP can run a secondary pass #222/Design: pre-ASAP structural CSE via bottom-up hash-consing over Rc<QueryExpr> #223). CSE's hash-consing already decides — correctly and conservatively, gated onSchema::unique_keys— which subtrees are safe to share across a workload. Two or more workload locations ending up with the sameRc<QueryExpr>after that pass is the applicability finding for the catalog's "cross-statistic reuse" / "cross-metrics reuse" / "cross-subpopulation reuse" entries — they all reduce to the same structural question CSE already answers. Findings report the maximal shared subtree (the highest point sharing starts), not every subsumed descendant.Both rules and the top-level entry points have real unit tests (positive and negative) in the module, built with the same kind of hand-rolled
QueryExprfixturesasap-aware-mapping's ownbind.rs/boundary.rstests already use (this crate has no front-end dependency, so no parser-based fixtures are available).Catalog primitives deliberately left as future work
The internal catalog (
ProjectASAP/internal-docs/catalog_of_optimizations.md) lists several primitives/optimizations with no implementation anywhere in this codebase today. None of these get a rule — faking one would report a finding the codebase can't back up. Full closure of #33 needs each underlying primitive to exist first; this PR's module doc has the detail, summarized here:Implementation::Wavelet/WaveletKindtypes already exist but no coreAggIntentdispatch orCostModel::realize_extensionpicks them yetImplementation::Sample/SamplingKindexist, unreachable from core dispatchImplementation/SummaryFamilyTypevariant exists at all yetQueryExpr::Scan'sSourcehas no pre-aggregated/rollup leaf variant yetTimeRange/PromqlSubquerywindows are always evaluated exactly todayRepeatingEntry/RepetitionIntervaldescribe that a query repeats, not any monitoring-specific decompositionRepeatingEntryevaluations todayAggIntent::Delta/IDeltaare PromQL value-difference semantics, not wire/storage delta encoding;DataDistribution's doc already discusses delta-compression benefit but nothing computes/applies it — this would be a deployment-side (post-ASAP) encoding decision, not IR-level dispatchChecks
cargo build --workspace --all-targets,cargo test --workspace,cargo fmt --all -- --check, andcargo clippy --workspace --all-targets --all-features -- -D warningsall pass clean.🤖 Generated with Claude Code