From 0e7621e9cc390c02c35981809977e3dd5fa27ac6 Mon Sep 17 00:00:00 2001 From: zz_y Date: Sat, 22 Aug 2026 14:39:46 -0600 Subject: [PATCH] docs(asap-aware-mapping): correct module doc on where CSE detection runs Records the placement decision settled in #222: common sub-expression elimination detection is a primary pass over the pre-ASAP QueryExpr IR (asap-types::pre_asap, design tracked in #223), not something asap-aware-mapping hoists after binding. This crate's module doc previously claimed "which shared sub-expressions to hoist" as its own job, but no such pass existed here or was ever meant to be primary. asap-aware-mapping may still eventually run a secondary, narrower CSE pass over already-implement_tree'd SummaryNodes, recognizing sharing that's invisible at the pre-ASAP level (e.g. two different accuracy targets on the same Quantile can share one built sketch) -- but that is downstream of, not a substitute for, the primary pre-ASAP pass. Part of #222 Co-Authored-By: Claude Sonnet 5 --- crates/asap-aware-mapping/src/lib.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/asap-aware-mapping/src/lib.rs b/crates/asap-aware-mapping/src/lib.rs index 77323ec0..31599363 100644 --- a/crates/asap-aware-mapping/src/lib.rs +++ b/crates/asap-aware-mapping/src/lib.rs @@ -3,8 +3,22 @@ //! This crate sits between the language-agnostic IR ([`asap_ir`]) and //! any runtime: it consumes pre-ASAP [`QueryExpr`](asap_types::pre_asap::QueryExpr) //! trees and makes the cost-aware decisions the pre-ASAP IR deliberately -//! leaves open — which shared sub-expressions to hoist and which sketch (if -//! any) realises each approximate intent. +//! leaves open — which sketch (if any) realises each approximate intent. +//! +//! **Common sub-expression elimination (CSE) is not this crate's job.** +//! Detection is a primary pass over the pre-ASAP `QueryExpr` IR itself +//! (`asap_types::pre_asap`, design tracked in issue #223), run before a +//! tree ever reaches [`bind::implement_tree`] — see issue #222 for why +//! (batch query optimization needs to see shared work across a +//! `QueryWorkload` before summary binding, not after). This crate may +//! eventually run a second, narrower CSE pass of its own over an +//! already-[`implement_tree`](bind::implement_tree)'d +//! `SummaryExpr`/`SummaryNode` DAG, recognizing sharing that's invisible +//! at the pre-ASAP level by construction — e.g. `Quantile(x, 0.99)` and +//! `Quantile(x, 0.95)` are structurally distinct `AggIntent`s but can +//! still share one built sketch, read out twice. That post-ASAP pass is +//! secondary to, and downstream of, the primary pre-ASAP pass, not a +//! replacement for it. //! //! It depends only on the IR crate, never on a front end — the layering //! invariant (arrows point up) holds here too.