From bb5c4cb009de5071f631b00385f602d151539d34 Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Fri, 14 Aug 2026 13:38:45 -0400 Subject: [PATCH] =?UTF-8?q?Remove=20QueryExpr::Ref=20/=20LetBinding=20?= =?UTF-8?q?=E2=80=94=20no=20producer=20exists=20(#181)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both variants (L3 in crates/ir, plus their L2 mirror in crates/l2's relational::QueryExpr) were marked Reserved: no front end ever emits them, and lowering every query across all 7 corpora we have confirms neither variant is reachable, including the SQL-CTE case they existed for (DataFusion inlines WITH bodies at every use site instead of sharing them). The only code that ever constructed a Ref node was crates/plan/src/cse.rs (dedupe_subtrees), unwired scaffolding for issue #6 that's never called outside its own unit tests. Deleted it along with the support types that existed solely for it: BindingScope/BindingName/QueryId and schema::cse_reuse_is_legal/CseError. Collapsed QueryExpr::output_schema_in's scope threading (crates/plan/src/bind.rs's implement_tree_in/_with) back into a plain output_schema(), since there's no longer a scope to resolve. Co-Authored-By: Claude Sonnet 5 --- crates/frontend-promql/src/promql.rs | 5 +- .../awesome_prometheus_alerts.rs | 9 +- .../tests/promql_conformance.rs | 9 +- crates/frontend-sql/src/sql/mod.rs | 5 +- .../synthetic_packet_trace.rs | 12 +- crates/frontend-sql/tests/netflow/netflow.rs | 9 +- crates/frontend-sql/tests/sql_lowering.rs | 5 +- crates/ir/src/dag_export.rs | 34 +-- crates/ir/src/intent_algebra/mod.rs | 11 +- crates/ir/src/intent_algebra/names.rs | 27 -- crates/ir/src/intent_algebra/query_expr.rs | 82 ++---- crates/ir/src/intent_algebra/schema.rs | 147 +--------- crates/l2/src/canonicalize.rs | 3 +- crates/l2/src/lower.rs | 13 +- crates/l2/src/relational.rs | 25 +- crates/lower/src/bin/variant_coverage.rs | 10 - crates/plan/src/bind.rs | 45 +-- crates/plan/src/cost_model.rs | 3 +- crates/plan/src/cse.rs | 259 ------------------ crates/plan/src/lib.rs | 9 +- crates/sketch/src/expr.rs | 3 +- 21 files changed, 66 insertions(+), 659 deletions(-) delete mode 100644 crates/ir/src/intent_algebra/names.rs delete mode 100644 crates/plan/src/cse.rs diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index 07866512..4b8109fb 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -601,8 +601,9 @@ fn walk_histogram(call: &Call) -> Result { /// (`HistogramQuantile`), native histograms / raw samples take the sketch-able /// `Quantile` (issues #43 / #79) — so the two functions cannot diverge. /// -/// The vector argument is lowered once per branch. That is a duplicated subtree -/// by construction; `plan::cse` hoists it back into a single producer. +/// The vector argument is lowered once per branch, duplicating the subtree — +/// a future workload-level reuse pass could hoist it back into a single +/// producer. /// /// Each branch aliases its value column to `value` rather than taking the /// intent-keyed name (`quantile_0_5`, `quantile_0_9`, …). `Merge` derives its diff --git a/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs b/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs index 649263ca..59ad64db 100644 --- a/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs +++ b/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs @@ -83,17 +83,10 @@ fn intents(e: &QueryExpr) -> Vec { go(rhs, out); } QueryExpr::Merge { children } => children.iter().for_each(|c| go(c, out)), - QueryExpr::LetBinding { expr, child, .. } => { - go(expr, out); - go(child, out); - } QueryExpr::VectorFromScalar(inner) | QueryExpr::ScalarFromVector(inner) => { go(inner, out) } - QueryExpr::Scan { .. } - | QueryExpr::Scalar(_) - | QueryExpr::EvalTime - | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::EvalTime => {} } } go(e, &mut out); diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 664a68d8..c0927467 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -92,17 +92,10 @@ fn collect(e: &QueryExpr, out: &mut Vec) { collect(right, out); } QueryExpr::Merge { children } => children.iter().for_each(|c| collect(c, out)), - QueryExpr::LetBinding { expr, child, .. } => { - collect(expr, out); - collect(child, out); - } QueryExpr::VectorFromScalar(inner) | QueryExpr::ScalarFromVector(inner) => { collect(inner, out) } - QueryExpr::Scan { .. } - | QueryExpr::Scalar(_) - | QueryExpr::EvalTime - | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::EvalTime => {} } } diff --git a/crates/frontend-sql/src/sql/mod.rs b/crates/frontend-sql/src/sql/mod.rs index 8ddcdd9d..f1457075 100644 --- a/crates/frontend-sql/src/sql/mod.rs +++ b/crates/frontend-sql/src/sql/mod.rs @@ -603,8 +603,9 @@ impl<'a> SqlLowerer<'a> { /// the branches union-compatible, which `Merge` requires (it derives its /// schema from the first child). /// - /// `Aggregate.child` is duplicated per level. `plan::cse` hoists it back into - /// a single producer — the same trade `histogram_quantiles` makes (#109). + /// `Aggregate.child` is duplicated per level — the same trade + /// `histogram_quantiles` makes (#109); a future workload-level reuse pass + /// could hoist it back into a single producer. /// /// DataFusion's `__grouping_id` discriminator is dropped: it only exists to /// tell a subtotal's `NULL` apart from a data `NULL`, which is observable diff --git a/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs b/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs index 1aab4f41..e5b16d36 100644 --- a/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs +++ b/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs @@ -105,17 +105,10 @@ fn intents(e: &QueryExpr) -> Vec { go(rhs, out); } QueryExpr::Merge { children } => children.iter().for_each(|c| go(c, out)), - QueryExpr::LetBinding { expr, child, .. } => { - go(expr, out); - go(child, out); - } QueryExpr::VectorFromScalar(inner) | QueryExpr::ScalarFromVector(inner) => { go(inner, out) } - QueryExpr::Scan { .. } - | QueryExpr::Scalar(_) - | QueryExpr::EvalTime - | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::EvalTime => {} } } go(e, &mut out); @@ -152,9 +145,6 @@ fn has_window_func(qe: &QueryExpr) -> bool { | QueryExpr::Sort { child, .. } | QueryExpr::Limit { child, .. } | QueryExpr::Subquery { child, .. } => has_window_func(child), - QueryExpr::LetBinding { expr, child, .. } => { - has_window_func(expr) || has_window_func(child) - } _ => false, } } diff --git a/crates/frontend-sql/tests/netflow/netflow.rs b/crates/frontend-sql/tests/netflow/netflow.rs index 187d138f..f4e1d89d 100644 --- a/crates/frontend-sql/tests/netflow/netflow.rs +++ b/crates/frontend-sql/tests/netflow/netflow.rs @@ -293,14 +293,7 @@ fn visit(qe: &QueryExpr, f: &mut impl FnMut(&QueryExpr)) { visit(child, f); } } - QueryExpr::LetBinding { expr, child, .. } => { - visit(expr, f); - visit(child, f); - } QueryExpr::VectorFromScalar(child) | QueryExpr::ScalarFromVector(child) => visit(child, f), - QueryExpr::Scan { .. } - | QueryExpr::Scalar(_) - | QueryExpr::EvalTime - | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::EvalTime => {} } } diff --git a/crates/frontend-sql/tests/sql_lowering.rs b/crates/frontend-sql/tests/sql_lowering.rs index d8e80279..385c6072 100644 --- a/crates/frontend-sql/tests/sql_lowering.rs +++ b/crates/frontend-sql/tests/sql_lowering.rs @@ -909,8 +909,9 @@ async fn a_correlated_in_subquery_is_rejected() { #[tokio::test] async fn quantile_carries_its_input_column() { // `metrics(ts=0, service=1, latency=2, bytes=3)`. Two quantiles over - // different columns must not compare equal — `plan::cse` dedupes on - // `AggIntent` equality, so a col-less intent would collapse them. + // different columns must not compare equal — a workload-level dedupe pass + // would compare on `AggIntent` equality, so a col-less intent would + // collapse them. let qe = lower( "SELECT approx_percentile_cont(latency, 0.5), \ approx_percentile_cont(bytes, 0.5) FROM metrics", diff --git a/crates/ir/src/dag_export.rs b/crates/ir/src/dag_export.rs index ef78b02a..a4c87b39 100644 --- a/crates/ir/src/dag_export.rs +++ b/crates/ir/src/dag_export.rs @@ -9,10 +9,7 @@ //! structural hash per node, so a caller with several exported queries can //! spot identical subtrees (a shared `Scan`, a repeated `Aggregate` shape, //! …) by comparing hashes rather than re-implementing `QueryExpr: PartialEq` -//! structural comparison client-side. This is a proxy for real CSE output -//! (`asap_plan::cse::dedupe_subtrees`), not a replacement for it — that pass -//! isn't wired into any end-to-end multi-root pipeline today, so its -//! `bindings`/`Ref` shape isn't something an exported single query exposes. +//! structural comparison client-side. use serde::Serialize; use std::hash::{Hash, Hasher}; @@ -31,7 +28,7 @@ pub struct DagNode { pub label: String, pub detail: serde_json::Value, /// Child node ids, in the variant's field order (e.g. `Join` is - /// `[left, right]`, `LetBinding` is `[expr, child]`). + /// `[left, right]`). pub children: Vec, /// Bottom-up structural hash: two nodes hash equally iff their `kind`, /// `detail`, and (recursively) their children's hashes all match. @@ -124,7 +121,7 @@ fn source_label(source: &Source) -> String { /// (children pushed before their parent), and return the id of the pushed /// root node. Exhaustive over every `QueryExpr` variant — a new variant /// fails to compile here until this match is extended, matching the rest of -/// the IR's exhaustive-match style (e.g. `output_schema_in`, `plan::cse`). +/// the IR's exhaustive-match style (e.g. `output_schema`). fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { match expr { QueryExpr::Scan { @@ -140,16 +137,6 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node(nodes, "Scan", label, detail, vec![]) } - QueryExpr::Ref { name } => { - let detail = serde_json::json!({ "name": name }); - push_node( - nodes, - "Ref", - format!("Ref({})", name.as_str()), - detail, - vec![], - ) - } QueryExpr::Scalar(v) => { let detail = serde_json::json!({ "value": v }); push_node(nodes, "Scalar", format!("Scalar({v})"), detail, vec![]) @@ -330,21 +317,6 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "n": n, "offset": offset }); push_node(nodes, "Limit", format!("Limit({n})"), detail, vec![c]) } - QueryExpr::LetBinding { name, expr, child } => { - // Two children with asymmetric roles: `expr` (the bound producer) - // and `child` (the body); order is [expr, child] so an index into - // `children` always means the same thing across `LetBinding` nodes. - let e = build(expr, nodes); - let c = build(child, nodes); - let detail = serde_json::json!({ "name": name }); - push_node( - nodes, - "LetBinding", - format!("LetBinding({})", name.as_str()), - detail, - vec![e, c], - ) - } QueryExpr::Subquery { range, resolution, diff --git a/crates/ir/src/intent_algebra/mod.rs b/crates/ir/src/intent_algebra/mod.rs index b693414a..6611bb52 100644 --- a/crates/ir/src/intent_algebra/mod.rs +++ b/crates/ir/src/intent_algebra/mod.rs @@ -7,16 +7,14 @@ //! - [`expr_ir`] — the scalar expression IR ([`L2Expr`] / [`L3Expr`] / //! [`ColumnRef`]) shared by L2 and L3. //! - [`schema`] — the per-edge [`Schema`] every L3 node carries. -//! - [`names`] — binding / query identifiers. //! //! The Layer-2 relational tree and the L2→L3 converter (`convert_root`, the //! `Binder`, column resolution) live in the `asap-l2` crate — front ends need //! them, but L3-only consumers (optimizer, sketch) do not, so they stay out of -//! this crate. Workload-level CSE lives in `asap-plan`. +//! this crate. pub mod agg_intent; pub mod expr_ir; -pub mod names; pub mod query_expr; pub mod schema; @@ -25,11 +23,10 @@ pub use agg_intent::{ is_frequency_heavy_hitter, ranking_measure, AggIntent, MathFunc, RankingMeasure, TimeFunc, }; pub use expr_ir::{ArithOp, ColumnRef, CompareOp, Expr, L2Expr, L3Expr, L3Scalar}; -pub use names::{BindingName, QueryId}; pub use query_expr::{ - aggregate_output_schema, AtModifier, BinaryOpKind, BindingScope, DataModel, GroupKeys, - GroupSide, InfoMatcher, JoinKind, Predicate, ProjectItem, QueryExpr, QueryExprError, Reduction, + aggregate_output_schema, AtModifier, BinaryOpKind, DataModel, GroupKeys, GroupSide, + InfoMatcher, JoinKind, Predicate, ProjectItem, QueryExpr, QueryExprError, Reduction, SampleKind, SetOpKind, SortKey, Source, TimeShift, VectorGrouping, VectorMatch, VectorMatchKind, WindowFuncKind, WindowKind, }; -pub use schema::{cse_reuse_is_legal, Column, ColumnId, CseError, DataType, Schema}; +pub use schema::{Column, ColumnId, DataType, Schema}; diff --git a/crates/ir/src/intent_algebra/names.rs b/crates/ir/src/intent_algebra/names.rs deleted file mode 100644 index 071bf905..00000000 --- a/crates/ir/src/intent_algebra/names.rs +++ /dev/null @@ -1,27 +0,0 @@ -use serde::{Deserialize, Serialize}; - -/// Name of a `LetBinding` / `Ref` sub-expression (CSE producer alias). -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -pub struct BindingName(pub String); - -impl BindingName { - pub fn new(s: impl Into) -> Self { - Self(s.into()) - } - pub fn as_str(&self) -> &str { - &self.0 - } -} - -/// Stable identifier for one query within a workload (CSE root key). -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -pub struct QueryId(pub String); - -impl QueryId { - pub fn new(s: impl Into) -> Self { - Self(s.into()) - } - pub fn as_str(&self) -> &str { - &self.0 - } -} diff --git a/crates/ir/src/intent_algebra/query_expr.rs b/crates/ir/src/intent_algebra/query_expr.rs index f4669f0e..372c4750 100644 --- a/crates/ir/src/intent_algebra/query_expr.rs +++ b/crates/ir/src/intent_algebra/query_expr.rs @@ -1,12 +1,10 @@ //! The canonical Layer-3 intent algebra IR. //! -//! Language- and deployment-independent. Box-owned tree (DAG fan-in is -//! expressed via `LetBinding` / `Ref`); column identity is **positional** +//! Language- and deployment-independent. Box-owned tree; column identity is **positional** //! (`Aggregate.reduction: Reduction`, wrapping `GroupKeys` for the //! grouped case), resolved by the [`Binder`](super::binder) against the //! self-contained [`Schema`] carried on each `Scan`. -use std::collections::HashMap; use std::time::Duration; use serde::{Deserialize, Serialize}; @@ -14,14 +12,11 @@ use thiserror::Error; use super::agg_intent::AggIntent; use super::expr_ir::{ArithOp, CompareOp, L3Expr, L3Scalar}; -use super::names::BindingName; use super::schema::{Column, ColumnId, DataType, Schema}; /// Errors from schema derivation over a canonical tree. #[derive(Debug, Error)] pub enum QueryExprError { - #[error("unresolved ref: {0}")] - UnresolvedRef(String), #[error("by-column id {0} out of range (input has {1} columns)")] InvalidGroupByColumn(ColumnId, usize), #[error("Merge requires at least one child")] @@ -492,9 +487,6 @@ pub enum QueryExpr { predicates: Vec, schema: Schema, }, - /// Reference to a `LetBinding` by name; resolved at plan time. - Ref { name: BindingName }, - /// A scalar constant leaf — a PromQL number literal or a folded constant /// scalar expression (`10*1024*1024`). Appears as a [`BinaryOp`](Self::BinaryOp) /// operand for ` op ` thresholds / unit conversions (#35). @@ -663,13 +655,6 @@ pub enum QueryExpr { child: Box, }, - /// SQL `WITH name AS (expr) … child`; PromQL recording-rule binding. - LetBinding { - name: BindingName, - expr: Box, - child: Box, - }, - /// PromQL sub-query (`[range:resolution]`). Logical pass-through. Subquery { range: Duration, @@ -731,13 +716,8 @@ pub enum QueryExpr { } impl QueryExpr { - /// Output schema of the root of a single query (empty binding scope). + /// Output schema of the root of a canonical tree. pub fn output_schema(&self) -> Result { - self.output_schema_in(&BindingScope::default()) - } - - /// Output schema given an explicit `LetBinding` scope. - pub fn output_schema_in(&self, scope: &BindingScope) -> Result { match self { QueryExpr::Scan { schema, .. } => Ok(schema.clone()), @@ -745,7 +725,7 @@ impl QueryExpr { // repetition. Does not change the column schema; passes through. // Per-series range reductions (`rate`, `*_over_time`) now use the // `TimeRange` node instead, so this arm is a simple pass-through. - QueryExpr::Window { child, .. } => child.output_schema_in(scope), + QueryExpr::Window { child, .. } => child.output_schema(), QueryExpr::Aggregate { reduction, @@ -754,20 +734,10 @@ impl QueryExpr { child, .. } => { - let in_schema = child.output_schema_in(scope)?; + let in_schema = child.output_schema()?; aggregate_output_schema(&in_schema, reduction, aggs, output_names) } - QueryExpr::LetBinding { name, expr, child } => { - let bound = expr.output_schema_in(scope)?; - let extended = scope.with(name.clone(), bound); - child.output_schema_in(&extended) - } - QueryExpr::Ref { name } => scope - .lookup(name) - .cloned() - .ok_or_else(|| QueryExprError::UnresolvedRef(name.as_str().into())), - QueryExpr::Filter { child, .. } | QueryExpr::Sort { child, .. } | QueryExpr::Limit { child, .. } @@ -781,7 +751,7 @@ impl QueryExpr { | QueryExpr::TimeRange { child, .. } // A time shift (`offset`/`@`) moves *when* the child is evaluated, // never its columns — schema passes through (#40). - | QueryExpr::TimeShift { child, .. } => child.output_schema_in(scope), + | QueryExpr::TimeShift { child, .. } => child.output_schema(), // ρ — relabel preserves every input column and writes one label // `dst` (Utf8): overwritten in place if it already exists, else @@ -790,7 +760,7 @@ impl QueryExpr { // A rewrite can collapse two label sets into one, so row-uniqueness // is no longer provable — drop unique_keys. QueryExpr::Relabel { dst, child, .. } => { - let mut out = child.output_schema_in(scope)?; + let mut out = child.output_schema()?; if let Some(existing) = out.columns.iter_mut().find(|c| c.name == *dst) { existing.dtype = DataType::Utf8; existing.nullable = true; @@ -807,7 +777,7 @@ impl QueryExpr { // the grouping/time columns, so unique_keys reset and time_index // is re-found by name. QueryExpr::Project { cols, qualifier, child } => { - let in_schema = child.output_schema_in(scope)?; + let in_schema = child.output_schema()?; let columns: Vec = cols .iter() .enumerate() @@ -838,7 +808,7 @@ impl QueryExpr { } QueryExpr::Distinct { cols, child } => { - let mut out = child.output_schema_in(scope)?; + let mut out = child.output_schema()?; // Deduplicating on `cols` makes them a unique key of the result. if !cols.is_empty() { out.add_unique_key(cols.clone()); @@ -854,7 +824,7 @@ impl QueryExpr { let mut s = children .first() .ok_or(QueryExprError::EmptyMerge) - .and_then(|c| c.output_schema_in(scope))?; + .and_then(|c| c.output_schema())?; s.unique_keys.clear(); Ok(s) } @@ -862,7 +832,7 @@ impl QueryExpr { // column shape, so the output schema is the left's. (Row identity // is not preserved across a UNION, so unique_keys are dropped.) QueryExpr::SetOp { left, .. } => { - let mut s = left.output_schema_in(scope)?; + let mut s = left.output_schema()?; s.unique_keys.clear(); Ok(s) } @@ -872,8 +842,8 @@ impl QueryExpr { QueryExpr::Join { kind, left, right, .. } => { - let l = left.output_schema_in(scope)?; - let r = right.output_schema_in(scope)?; + let l = left.output_schema()?; + let r = right.output_schema()?; // Semi / anti joins filter the left side; the right contributes // no columns, so the output is the left's schema unchanged. Row // identity *is* preserved (each left row appears at most once), @@ -918,7 +888,7 @@ impl QueryExpr { child, .. } => { - let mut out = child.output_schema_in(scope)?; + let mut out = child.output_schema()?; // First operand's (dtype, nullable) from the child schema, owned // so the borrow ends before we append. let arg = args.first().and_then(|a| match a { @@ -988,8 +958,8 @@ impl QueryExpr { ( QueryExpr::Scalar(_) | QueryExpr::EvalTime | QueryExpr::ScalarFromVector(_), r, - ) => r.output_schema_in(scope), - (l, _) => l.output_schema_in(scope), + ) => r.output_schema(), + (l, _) => l.output_schema(), }, } } @@ -1027,7 +997,7 @@ fn per_series_reduction_schema(input: &Schema, agg: &AggIntent) -> Schema { /// The output schema of an `Aggregate { reduction, aggs }` over `in_schema` — /// the **single** canonical derivation shared by -/// [`QueryExpr::output_schema_in`]'s `Aggregate` arm and the converter's +/// [`QueryExpr::output_schema`]'s `Aggregate` arm and the converter's /// HAVING-resolution path (`column_resolution::output_schema_for_aggregate`), /// so the two can never drift (issue #41). /// @@ -1250,26 +1220,6 @@ fn default_proj_name(expr: &L3Expr, idx: usize, schema: &Schema) -> String { } } -/// Lexical scope for `LetBinding` / `Ref` resolution. -#[derive(Debug, Default, Clone)] -pub struct BindingScope { - bindings: HashMap, -} - -impl BindingScope { - pub fn new() -> Self { - Self::default() - } - pub fn with(&self, name: BindingName, schema: Schema) -> Self { - let mut bindings = self.bindings.clone(); - bindings.insert(name.as_str().into(), schema); - Self { bindings } - } - pub fn lookup(&self, name: &BindingName) -> Option<&Schema> { - self.bindings.get(name.as_str()) - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/ir/src/intent_algebra/schema.rs b/crates/ir/src/intent_algebra/schema.rs index fa0b68a8..46db0eb7 100644 --- a/crates/ir/src/intent_algebra/schema.rs +++ b/crates/ir/src/intent_algebra/schema.rs @@ -5,11 +5,9 @@ //! function of its inputs and parameters and is verifiable independently //! of the surrounding context. //! -//! `Schema::unique_keys` is the load-bearing field for the workload-level -//! CSE pass (`design.md` §6 "DAG, not tree" + the batched-queries example -//! around line ~1284). Two `QueryExpr::Ref` consumers can share a producer -//! only when its output schema is provably stable across reads — the -//! unique-key metadata is what lets the deduper assert that. +//! `Schema::unique_keys` is metadata for reuse-aware planning: a producer's +//! output can only be safely shared across consumers when its row identity +//! is provably stable across reads, which is what this field records. //! //! Single-query plans don't read this field; it lives here so the metadata //! is available the moment workload-aware planning lands without requiring @@ -97,9 +95,9 @@ pub enum DataType { /// `Aggregate { by, .. }` emits `unique_keys = [by]`; `Distinct { cols }` /// adds `cols`; most other nodes pass through. /// -/// **Consumed by**: workload-level CSE (`CostModel::workload_cost` in the -/// design, not yet shipped). The single-query path, the `Bind*` rules, -/// push-down, and L5 emitters do not read this field. +/// **Consumed by**: a future workload-level reuse pass (not yet shipped). +/// The single-query path, the `Bind*` rules, push-down, and L5 emitters do +/// not read this field. #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize)] pub struct Schema { /// Columns flowing on this edge, in positional order. @@ -175,10 +173,9 @@ impl Schema { .position(|c| c.name == name && c.table.as_deref() == Some(table)) } - /// Whether this schema has *any* provable unique key. The CSE pass - /// reads this to decide whether two `Ref` consumers can safely share - /// a producer (see `design.md` §6 line ~1284 + the unit test in - /// `tests::cse_substitution_legal_only_with_unique_keys`). + /// Whether this schema has *any* provable unique key — the signal a + /// future reuse-aware planning pass would need to decide whether a + /// producer's output can be safely shared across consumers. pub fn has_unique_key(&self) -> bool { !self.unique_keys.is_empty() } @@ -193,68 +190,6 @@ impl Schema { } } -// ── CSE legality (the load-bearing consumer of `unique_keys`) ──────────────── -// -// Phase F per `control_plane/docs/design.md` §6 Schema flow + the batched- -// queries example (§6 line ~1320): -// -// "CSE legality leans on `Schema::unique_keys` (§6 Schema flow): two -// `QueryExpr::Ref` consumers can share a producer only when its -// output schema is provably stable across reads — the unique-key -// metadata is what lets the deduper assert that without re-running -// the producer's logic." -// -// `cse_reuse_is_legal` is the gatekeeper. The workload-level CSE pass -// (`asap_plan::cse::dedupe_subtrees`) consults it before emitting a -// `LetBinding` to share a producer between ≥2 `Ref` consumers. - -use thiserror::Error; - -/// Errors returned by [`cse_reuse_is_legal`] when shared-producer reuse -/// would violate the design's stability invariant. -#[derive(Debug, Error, PartialEq, Eq)] -pub enum CseError { - /// Producer schema lacks any `unique_keys` set — row identity is - /// not provably stable across reads, so two `Ref` consumers cannot - /// safely share it. The deduper falls back to per-consumer - /// recomputation. Per design.md §6 line ~1356. - #[error( - "shared-producer CSE refused: producer schema has no unique_keys \ - (design.md §6 schema-flow — without a provable unique key the \ - deduper cannot assert row identity across reads)" - )] - NoUniqueKeys, - /// Trivially-callable case: only one consumer means no reuse to - /// gate. Returned so the caller can short-circuit instead of - /// emitting a degenerate `LetBinding`. - #[error("CSE not applicable: {0} consumer(s) — need ≥ 2 for shared-producer reuse")] - InsufficientConsumers(usize), -} - -/// Two `QueryExpr::Ref` nodes can share a producer (same `LetBinding`) -/// only when the producer's output schema has stable per-row identity — -/// i.e. `Schema::unique_keys` is non-empty. This is the gatekeeper: -/// returns `Ok(())` if shared-producer reuse is legal, otherwise `Err`. -/// -/// Per design.md §6 line ~1356 — `unique_keys` is what makes CSE -/// provably correct. The deduper consults this before emitting a -/// `LetBinding`, and `CostModel::workload_cost` only credits a shared -/// binding when this gate has fired green. -/// -/// `consumer_count` is the number of `QueryExpr::Ref { name }` sites the -/// deduper has identified for the candidate binding. Single-consumer -/// cases short-circuit with `InsufficientConsumers` — a `LetBinding` -/// with one `Ref` is just a no-op alias and shouldn't be hoisted. -pub fn cse_reuse_is_legal(producer_schema: &Schema, consumer_count: usize) -> Result<(), CseError> { - if consumer_count < 2 { - return Err(CseError::InsufficientConsumers(consumer_count)); - } - if !producer_schema.has_unique_key() { - return Err(CseError::NoUniqueKeys); - } - Ok(()) -} - // ── Tests ───────────────────────────────────────────────────────────────────── #[cfg(test)] @@ -265,70 +200,6 @@ mod tests { Column::new(name, dtype, false) } - /// `cse_reuse_is_legal` accepts a producer schema with at least one - /// `unique_keys` set + ≥2 consumers. This is the design.md §6 - /// "load-bearing" green path. - #[test] - fn cse_reuse_legal_when_unique_keys_set() { - let producer = Schema::with_time_index( - vec![ - col("ts", DataType::Timestamp), - col("service", DataType::Utf8), - col("value", DataType::Float64), - ], - 0, - vec![vec![0, 1]], - ); - assert_eq!(cse_reuse_is_legal(&producer, 2), Ok(())); - assert_eq!(cse_reuse_is_legal(&producer, 5), Ok(())); - } - - /// Schema without `unique_keys` is the conservative-default case — - /// the deduper must refuse to share it. Pins design.md §6 line - /// ~1356 ("Without it, the deduper has to be conservative and reuse - /// drops on the floor"). - #[test] - fn cse_reuse_illegal_when_unique_keys_empty() { - let producer = Schema::new(vec![col("a", DataType::Int64), col("b", DataType::Float64)]); - assert_eq!( - cse_reuse_is_legal(&producer, 2), - Err(CseError::NoUniqueKeys) - ); - } - - /// Single-consumer case is short-circuited — no `LetBinding` should - /// be emitted for one `Ref` because there's no reuse to credit. - #[test] - fn cse_reuse_rejects_single_consumer() { - let producer = Schema::with_time_index( - vec![col("ts", DataType::Timestamp), col("v", DataType::Float64)], - 0, - vec![vec![0]], - ); - assert_eq!( - cse_reuse_is_legal(&producer, 1), - Err(CseError::InsufficientConsumers(1)) - ); - assert_eq!( - cse_reuse_is_legal(&producer, 0), - Err(CseError::InsufficientConsumers(0)) - ); - } - - /// Empty `unique_keys` rejection takes precedence over the consumer - /// count check only when both pass — but here we verify the - /// insufficient-consumers branch fires first (a defensive ordering - /// so callers see the clearer error when they get the call wrong). - #[test] - fn cse_reuse_consumer_check_precedes_unique_key_check() { - let producer = Schema::new(vec![col("a", DataType::Int64)]); - // Both conditions fail; consumer check is reported. - assert_eq!( - cse_reuse_is_legal(&producer, 1), - Err(CseError::InsufficientConsumers(1)) - ); - } - #[test] fn schema_new_has_no_time_or_unique_key() { let s = Schema::new(vec![col("k", DataType::Utf8), col("v", DataType::Float64)]); diff --git a/crates/l2/src/canonicalize.rs b/crates/l2/src/canonicalize.rs index cc8b9532..cc9e28bc 100644 --- a/crates/l2/src/canonicalize.rs +++ b/crates/l2/src/canonicalize.rs @@ -59,7 +59,7 @@ fn canon(expr: &mut QueryExpr) { fn children_mut(expr: &mut QueryExpr) -> Vec<&mut QueryExpr> { use QueryExpr::*; match expr { - Scan { .. } | Ref { .. } | Scalar(_) | EvalTime => vec![], + Scan { .. } | Scalar(_) | EvalTime => vec![], VectorFromScalar(c) | ScalarFromVector(c) => vec![c.as_mut()], Relabel { child, .. } | Filter { child, .. } @@ -75,7 +75,6 @@ fn children_mut(expr: &mut QueryExpr) -> Vec<&mut QueryExpr> { | InfoJoin { child, .. } | Sort { child, .. } | Limit { child, .. } => vec![child.as_mut()], - LetBinding { expr, child, .. } => vec![expr.as_mut(), child.as_mut()], Merge { children } => children.iter_mut().collect(), Join { left, right, .. } | SetOp { left, right, .. } => { vec![left.as_mut(), right.as_mut()] diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index ed812990..0c62b0d4 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -22,7 +22,6 @@ use crate::column_resolution::{ use crate::relational::{AggFunc, QueryExpr as LQueryExpr, SourceSpec}; use asap_ir::intent_algebra::agg_intent::AggIntent; use asap_ir::intent_algebra::expr_ir::{ColumnRef, L2Expr, L3Expr, L3Scalar}; -use asap_ir::intent_algebra::names::BindingName; use asap_ir::intent_algebra::query_expr::{ GroupKeys, Predicate, ProjectItem, QueryExpr as CQueryExpr, Reduction, SortKey, Source, }; @@ -149,10 +148,6 @@ pub fn convert( child: Box::new(convert(input, fallback, acc)?), }, - LQueryExpr::Ref(name) => CQueryExpr::Ref { - name: BindingName::new(name.clone()), - }, - // Fold label matchers / pushed-down predicates directly onto the Scan // when the immediate child is a `Source`; otherwise emit a `Filter`. // Predicate column refs resolve positionally against the input schema. @@ -178,7 +173,7 @@ pub fn convert( // Single-statistic aggregate (no HAVING) over a *time-series* leaf // fuses into the canonical shape: a `Window` input becomes // `Window { Aggregate { by: [] } }` (a per-series, label-preserving - // reduction — see `output_schema_in`'s `Window` arm). GROUP BY keys + // reduction — see `output_schema`'s `Window` arm). GROUP BY keys // resolve *positionally* into `Aggregate.by` — the same shape SQL // produces — whenever they're in scope: an instant selector, or a // label-preserving per-series `rate`/`increase`/`*_over_time`. A @@ -492,12 +487,6 @@ pub fn convert( child: Box::new(convert(input, fallback, acc)?), }, - LQueryExpr::LetBinding { name, expr, body } => CQueryExpr::LetBinding { - name: BindingName::new(name.clone()), - expr: Box::new(convert(expr, fallback, acc)?), - child: Box::new(convert(body, fallback, acc)?), - }, - LQueryExpr::PromQLSubquery { range, resolution, diff --git a/crates/l2/src/relational.rs b/crates/l2/src/relational.rs index 88a96399..4b3446d0 100644 --- a/crates/l2/src/relational.rs +++ b/crates/l2/src/relational.rs @@ -234,11 +234,6 @@ pub enum QueryExpr { selector: Vec, input: Box, }, - /// Reference to a CTE / let-binding by name. **Reserved**: no front end - /// emits `Ref`/`LetBinding` yet (CSE runs on L3); the converter arm exists - /// for forward-compatibility (e.g. PromQL recording rules). - Ref(String), - /// σ — row-level filter (WHERE / PromQL label matchers). Filter { pred: L2Expr, input: Box }, @@ -328,14 +323,6 @@ pub enum QueryExpr { input: Box, }, - /// **Reserved**: see [`Ref`](Self::Ref) — no front end emits `LetBinding` - /// yet (DAG fan-in / CSE is expressed on L3). - LetBinding { - name: String, - expr: Box, - body: Box, - }, - /// PromQL sub-query syntax: `[range:resolution]`. PromQLSubquery { range: Duration, @@ -367,10 +354,7 @@ impl QueryExpr { pub fn walk(&self, f: &mut F) { f(self); match self { - QueryExpr::Source(_) - | QueryExpr::Scalar(_) - | QueryExpr::EvalTime - | QueryExpr::Ref(_) => {} + QueryExpr::Source(_) | QueryExpr::Scalar(_) | QueryExpr::EvalTime => {} QueryExpr::Filter { input, .. } | QueryExpr::Project { input, .. } | QueryExpr::Aggregate { input, .. } @@ -401,10 +385,6 @@ impl QueryExpr { left.walk(f); right.walk(f); } - QueryExpr::LetBinding { expr, body, .. } => { - expr.walk(f); - body.walk(f); - } } } @@ -431,8 +411,7 @@ impl QueryExpr { QueryExpr::Join { left, .. } | QueryExpr::SetOp { left, .. } | QueryExpr::BinaryOp { lhs: left, .. } => left.leaf_source(), - QueryExpr::LetBinding { body, .. } => body.leaf_source(), - QueryExpr::Scalar(_) | QueryExpr::EvalTime | QueryExpr::Ref(_) => None, + QueryExpr::Scalar(_) | QueryExpr::EvalTime => None, } } diff --git a/crates/lower/src/bin/variant_coverage.rs b/crates/lower/src/bin/variant_coverage.rs index 5b040b9e..e0843a81 100644 --- a/crates/lower/src/bin/variant_coverage.rs +++ b/crates/lower/src/bin/variant_coverage.rs @@ -14,7 +14,6 @@ use std::collections::BTreeSet; const ALL_VARIANTS: &[&str] = &[ "Scan", - "Ref", "Scalar", "EvalTime", "VectorFromScalar", @@ -32,7 +31,6 @@ const ALL_VARIANTS: &[&str] = &[ "SetOp", "Sort", "Limit", - "LetBinding", "Subquery", "TimeRange", "TimeShift", @@ -45,9 +43,6 @@ fn walk(e: &QueryExpr, seen: &mut BTreeSet<&'static str>) { QueryExpr::Scan { .. } => { seen.insert("Scan"); } - QueryExpr::Ref { .. } => { - seen.insert("Ref"); - } QueryExpr::Scalar(_) => { seen.insert("Scalar"); } @@ -116,11 +111,6 @@ fn walk(e: &QueryExpr, seen: &mut BTreeSet<&'static str>) { seen.insert("Limit"); walk(child, seen); } - QueryExpr::LetBinding { expr, child, .. } => { - seen.insert("LetBinding"); - walk(expr, seen); - walk(child, seen); - } QueryExpr::Subquery { child, .. } => { seen.insert("Subquery"); walk(child, seen); diff --git a/crates/plan/src/bind.rs b/crates/plan/src/bind.rs index 78e167d7..46678554 100644 --- a/crates/plan/src/bind.rs +++ b/crates/plan/src/bind.rs @@ -33,7 +33,7 @@ use std::rc::Rc; use asap_ir::intent_algebra::agg_intent::AggIntent; use asap_ir::intent_algebra::expr_ir::ColumnRef; -use asap_ir::intent_algebra::query_expr::{BindingScope, QueryExpr, QueryExprError, Reduction}; +use asap_ir::intent_algebra::query_expr::{QueryExpr, QueryExprError, Reduction}; use asap_ir::intent_algebra::schema::Schema; use asap_sketch::{ L4DataType, L4Field, L4Node, L4Schema, SketchQuery, SummaryExpr, SummaryKind, SummaryParams, @@ -51,21 +51,12 @@ pub enum ImplementError { Schema(#[from] QueryExprError), } -/// Bind a single query (empty `LetBinding` scope) to the L4 IR. Ranks -/// candidate summaries via [`DefaultCostModel`] (`asap-plan`'s built-in -/// static preference order, unchanged); use [`implement_tree_with`] to plug in a -/// deployment-specific [`CostModel`] instead. +/// Bind a single query to the L4 IR. Ranks candidate summaries via +/// [`DefaultCostModel`] (`asap-plan`'s built-in static preference order, +/// unchanged); use [`implement_tree_with`] to plug in a deployment-specific +/// [`CostModel`] instead. pub fn implement_tree(expr: &QueryExpr) -> Result, ImplementError> { - implement_tree_in(expr, &BindingScope::default()) -} - -/// Bind with an explicit `LetBinding` scope — for roots that reference -/// CSE-hoisted producers via [`QueryExpr::Ref`]. -pub fn implement_tree_in( - expr: &QueryExpr, - scope: &BindingScope, -) -> Result, ImplementError> { - implement_tree_in_with(expr, scope, &DefaultCostModel) + implement_tree_with(expr, &DefaultCostModel) } /// Like [`implement_tree`], but ranks candidate summaries via `cost_model` (see @@ -73,15 +64,6 @@ pub fn implement_tree_in( pub fn implement_tree_with( expr: &QueryExpr, cost_model: &dyn CostModel, -) -> Result, ImplementError> { - implement_tree_in_with(expr, &BindingScope::default(), cost_model) -} - -/// Like [`implement_tree_in`], but ranks candidate summaries via `cost_model`. -pub fn implement_tree_in_with( - expr: &QueryExpr, - scope: &BindingScope, - cost_model: &dyn CostModel, ) -> Result, ImplementError> { if let QueryExpr::Aggregate { reduction, @@ -98,14 +80,14 @@ pub fn implement_tree_in_with( Implementation::Summary { kind, params } => { let estimate = !kind.is_exact(); return bind_summary_agg( - expr, reduction, intent, child, kind, params, scope, estimate, cost_model, + expr, reduction, intent, child, kind, params, estimate, cost_model, ); } Implementation::PassThrough => {} } } } - logical(expr, scope) + logical(expr) } /// Emit `SummaryAgg` (recursively binding the child), plus the @@ -118,11 +100,10 @@ fn bind_summary_agg( child: &QueryExpr, kind: SummaryKind, params: SummaryParams, - scope: &BindingScope, estimate: bool, cost_model: &dyn CostModel, ) -> Result, ImplementError> { - let child_schema = child.output_schema_in(scope)?; + let child_schema = child.output_schema()?; // The single canonical L3 derivation (per-series vs cross-series, name // overrides) already computes the row shape; L4 only retypes the summary // state column. @@ -131,7 +112,7 @@ fn bind_summary_agg( .group_keys() .map(|g| g.to_vec()) .unwrap_or_default(); - let out_schema = node.output_schema_in(scope)?; + let out_schema = node.output_schema()?; let state_idx = summary_col_index(&out_schema, &by, per_series); let col = summarised_column(intent, &child_schema); @@ -149,7 +130,7 @@ fn bind_summary_agg( // place that decides this; nothing downstream re-derives it. let agg = Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { - child: implement_tree_in_with(child, scope, cost_model)?, + child: implement_tree_with(child, cost_model)?, summary: kind, params, col, @@ -235,8 +216,8 @@ fn readout(intent: &AggIntent, col: &ColumnRef, cost_model: &dyn CostModel) -> S /// accumulator kind the deployment's runtime doesn't actually implement — /// through the same fallback this crate's own dispatch uses, without /// duplicating the schema-lift logic. -pub fn logical(expr: &QueryExpr, scope: &BindingScope) -> Result, ImplementError> { - let schema = expr.output_schema_in(scope)?; +pub fn logical(expr: &QueryExpr) -> Result, ImplementError> { + let schema = expr.output_schema()?; Ok(Rc::new(L4Node { expr: SummaryExpr::Logical(Box::new(expr.clone())), schema: lift(&schema), diff --git a/crates/plan/src/cost_model.rs b/crates/plan/src/cost_model.rs index ec9405e7..0ab0b7c7 100644 --- a/crates/plan/src/cost_model.rs +++ b/crates/plan/src/cost_model.rs @@ -22,8 +22,7 @@ //! //! Every entry point that doesn't take an explicit `&dyn CostModel` //! ([`implementation_for`](crate::boundary::implementation_for), -//! [`implement_tree`](crate::bind::implement_tree), -//! [`implement_tree_in`](crate::bind::implement_tree_in)) runs against +//! [`implement_tree`](crate::bind::implement_tree)) runs against //! [`DefaultCostModel`], so a deployment that never plugs in its own cost //! model keeps today's static-preference-order behavior exactly, byte for //! byte. diff --git a/crates/plan/src/cse.rs b/crates/plan/src/cse.rs deleted file mode 100644 index 7f71a7a8..00000000 --- a/crates/plan/src/cse.rs +++ /dev/null @@ -1,259 +0,0 @@ -//! Workload-level Common Sub-Expression Elimination. -//! -//! Multi-root planning hoists shared sub-DAGs into `LetBinding`s so the cost -//! model can credit the producer once. Legality is gated by -//! [`cse_reuse_is_legal`](asap_ir::intent_algebra::schema::cse_reuse_is_legal): a candidate -//! sub-DAG only becomes a binding when its output schema has at least one -//! `unique_keys` set — the load-bearing field for this pass. -//! -//! Scope: the basic "≥2 roots with identical `Aggregate`-child sub-trees" -//! case. The fully-general algorithm (alpha-equivalence, schema-merge, -//! nested CSE) is a downstream optimisation, not part of the IR contract. - -use asap_ir::intent_algebra::names::{BindingName, QueryId}; -use asap_ir::intent_algebra::query_expr::QueryExpr; -use asap_ir::intent_algebra::schema::cse_reuse_is_legal; - -/// Multi-root container produced by the CSE pass. -#[derive(Debug, Clone, PartialEq)] -pub struct CseWorkloadPlan { - /// Named shared producers, each referenced by ≥2 roots via `QueryExpr::Ref`. - pub bindings: Vec<(BindingName, QueryExpr)>, - /// One root per input query, in input order. - pub roots: Vec<(QueryId, QueryExpr)>, -} - -/// Hoist sub-expressions structurally identical across ≥2 roots into shared -/// `LetBinding`s, leaving each root with a `Ref` where the duplicate lived. -/// -/// A candidate is hoisted only when -/// `cse_reuse_is_legal(&candidate.output_schema(), consumers)` returns `Ok`. -pub fn dedupe_subtrees(roots: Vec<(QueryId, QueryExpr)>) -> CseWorkloadPlan { - if roots.len() < 2 { - return CseWorkloadPlan { - bindings: vec![], - roots, - }; - } - - // Count `Aggregate`-child sub-trees that appear in ≥2 roots. Group by - // structural equality (`QueryExpr: PartialEq`) rather than `Debug` - // output: `{:?}` is not a guaranteed-injective, stable identity contract. - // The candidate set is one entry per distinct root child, so this linear - // scan is bounded by the number of distinct queries. - let mut candidate_counts: Vec<(QueryExpr, usize)> = Vec::new(); - for (_, root) in &roots { - if let QueryExpr::Aggregate { child, .. } = root { - if matches!(**child, QueryExpr::Ref { .. }) { - continue; - } - match candidate_counts - .iter_mut() - .find(|(e, _)| e == child.as_ref()) - { - Some(entry) => entry.1 += 1, - None => candidate_counts.push(((**child).clone(), 1)), - } - } - } - - // Pick the most-shared legal candidate (biggest reuse first). - let mut chosen: Option<(QueryExpr, usize)> = None; - for (expr, count) in candidate_counts.into_iter() { - if count < 2 { - continue; - } - let Ok(out_schema) = expr.output_schema() else { - continue; - }; - if cse_reuse_is_legal(&out_schema, count).is_err() { - continue; - } - match &chosen { - Some((_, best)) if *best >= count => {} - _ => chosen = Some((expr, count)), - } - } - - let Some((shared_expr, _)) = chosen else { - return CseWorkloadPlan { - bindings: vec![], - roots, - }; - }; - - let binding_name = BindingName::new("shared_0"); - let mut rewritten: Vec<(QueryId, QueryExpr)> = Vec::with_capacity(roots.len()); - for (qid, root) in roots { - let new_root = match root { - QueryExpr::Aggregate { - reduction, - aggs, - output_names, - having, - child, - } if *child == shared_expr => QueryExpr::Aggregate { - reduction, - aggs, - output_names, - having, - child: Box::new(QueryExpr::Ref { - name: binding_name.clone(), - }), - }, - other => other, - }; - rewritten.push((qid, new_root)); - } - - CseWorkloadPlan { - bindings: vec![(binding_name, shared_expr)], - roots: rewritten, - } -} - -#[cfg(test)] -mod tests { - use super::*; - use asap_ir::intent_algebra::agg_intent::AggIntent; - use asap_ir::intent_algebra::query_expr::{Reduction, Source, WindowKind}; - use asap_ir::intent_algebra::schema::{Column, DataType, Schema}; - use asap_ir::types::AccuracyTarget; - use std::time::Duration; - - fn col(name: &str, dtype: DataType) -> Column { - Column::new(name, dtype, false) - } - - fn ts_scan() -> QueryExpr { - QueryExpr::Scan { - source: Source::TimeSeries { - metric: "http_request_duration_seconds".into(), - }, - predicates: vec![], - schema: Schema::with_time_index( - vec![ - col("ts", DataType::Timestamp), - col("service", DataType::Utf8), - col("value", DataType::Float64), - ], - 0, - vec![vec![0, 1]], - ), - } - } - - fn windowed_scan() -> QueryExpr { - QueryExpr::Window { - kind: WindowKind::Sliding, - size: Duration::from_secs(300), - slide: None, - child: Box::new(ts_scan()), - } - } - - #[test] - fn dedupe_subtrees_single_root_passthrough() { - let q = QueryExpr::Aggregate { - reduction: Reduction::by(vec![1]), - aggs: vec![AggIntent::Quantile { - col: None, - q: 0.99, - accuracy: AccuracyTarget::Epsilon(0.01), - }], - output_names: vec![], - having: None, - child: Box::new(windowed_scan()), - }; - let out = dedupe_subtrees(vec![(QueryId::new("q1"), q.clone())]); - assert!(out.bindings.is_empty()); - assert_eq!(out.roots[0].1, q); - } - - /// Issue #115: CSE dedupes on `AggIntent` equality. Before `Quantile` - /// carried its input column, `median(a)` and `median(b)` compared equal, so - /// two aggregates over *different* columns collapsed into one — a wrong - /// answer, not just a missed optimisation. - #[test] - fn quantiles_over_different_columns_do_not_dedupe() { - let mk = |col: usize| QueryExpr::Aggregate { - reduction: Reduction::by(vec![1]), - aggs: vec![AggIntent::Quantile { - col: Some(col), - q: 0.5, - accuracy: AccuracyTarget::Epsilon(0.01), - }], - output_names: vec![], - having: None, - child: Box::new(windowed_scan()), - }; - let (a, b) = (mk(2), mk(3)); - assert_ne!(a, b, "distinct-column quantiles must not compare equal"); - - // The shared scan is still hoisted; the Aggregate roots stay distinct. - let out = dedupe_subtrees(vec![(QueryId::new("q1"), a), (QueryId::new("q2"), b)]); - assert_ne!( - out.roots[0].1, out.roots[1].1, - "aggregates over different columns must not collapse" - ); - } - - #[test] - fn dedupe_subtrees_basic() { - let mk = |q: f64| QueryExpr::Aggregate { - reduction: Reduction::by(vec![1]), - aggs: vec![AggIntent::Quantile { - col: None, - q, - accuracy: AccuracyTarget::Epsilon(0.01), - }], - output_names: vec![], - having: None, - child: Box::new(windowed_scan()), - }; - let out = dedupe_subtrees(vec![ - (QueryId::new("q1"), mk(0.99)), - (QueryId::new("q2"), mk(0.95)), - ]); - assert_eq!(out.bindings.len(), 1); - assert_eq!(out.bindings[0].0, BindingName::new("shared_0")); - assert_eq!(out.bindings[0].1, windowed_scan()); - for (_, root) in &out.roots { - match root { - QueryExpr::Aggregate { child, .. } => assert_eq!( - **child, - QueryExpr::Ref { - name: BindingName::new("shared_0") - } - ), - other => panic!("expected Aggregate root, got {other:?}"), - } - } - } - - #[test] - fn dedupe_subtrees_no_shared_subexpr_when_unique_keys_absent() { - // Schema without unique_keys → CSE refuses to share even if identical. - let scan_no_uk = QueryExpr::Scan { - source: Source::TimeSeries { metric: "m".into() }, - predicates: vec![], - schema: Schema::with_time_index( - vec![ - col("ts", DataType::Timestamp), - col("value", DataType::Float64), - ], - 0, - vec![], - ), - }; - let mk = || QueryExpr::Aggregate { - reduction: Reduction::by(vec![]), - aggs: vec![AggIntent::Sum { col: None }], - output_names: vec![], - having: None, - child: Box::new(scan_no_uk.clone()), - }; - let out = dedupe_subtrees(vec![(QueryId::new("q1"), mk()), (QueryId::new("q2"), mk())]); - assert!(out.bindings.is_empty(), "no unique_keys → no hoisting"); - } -} diff --git a/crates/plan/src/lib.rs b/crates/plan/src/lib.rs index 011df46a..69ba0f13 100644 --- a/crates/plan/src/lib.rs +++ b/crates/plan/src/lib.rs @@ -15,9 +15,8 @@ //! //! ## Status //! -//! Three real occupants and one stub: +//! Two real occupants and one stub: //! -//! - [`cse`] — workload-level common-sub-expression elimination. //! - [`boundary`] — the per-intent sketch-vs-exact (accuracy) decision: //! `AggIntent → SummaryKind + SummaryParams` sized to the `AccuracyTarget` //! (issue #98). [`boundary::implementation_for`] is the per-node decision; @@ -68,15 +67,11 @@ //! `sketch_algebra::capability::Capability`/`is_satisfied_by` is the //! reference downstream implementation. -pub mod cse; - pub mod bind; pub mod boundary; pub mod cost_model; -pub use bind::{ - implement_tree, implement_tree_in, implement_tree_in_with, implement_tree_with, ImplementError, -}; +pub use bind::{implement_tree, implement_tree_with, ImplementError}; pub use boundary::{ implementation_for, implementation_for_with, summary_candidates, Implementation, Matcher, }; diff --git a/crates/sketch/src/expr.rs b/crates/sketch/src/expr.rs index e5885ac1..4cc30d89 100644 --- a/crates/sketch/src/expr.rs +++ b/crates/sketch/src/expr.rs @@ -25,8 +25,7 @@ pub struct L4Node { /// `Logical(Box)`. /// /// Traversing from the root node yields a DAG; shared sub-expressions appear -/// as multiple `Rc` references to the same `L4Node` (L3 fan-in is expressed -/// via `QueryExpr`'s own `LetBinding`/`Ref`). +/// as multiple `Rc` references to the same `L4Node`. #[derive(Debug, Clone)] pub enum SummaryExpr { /// Any L3 node that no L4 rule rewrote (e.g. `Filter`, `Project`, `Sort`).