refactor(sketch_algebra): 5-item touch-up — frequency split + Min/Max + dedupe (Task #32) - #133
Merged
Merged
Conversation
… + dedupe User-flagged cleanup of sketch_algebra/ after PR #130's layered refactor. ## 1. Drop `AggIntent::HistogramQuantile` `histogram_quantile()` is a PromQL/MetricsQL language-level operator, NOT a semantic intent. Removed the variant from `intent_algebra::agg_intent::AggIntent`. The PromQL operator still lives on as `intent_algebra::legacy_expr::QueryExpr::HistogramQuantile` (unreachable from canonical L3 — it was never produced by the lowerer, but PR #130's legacy_expr preservation keeps it for back-compat until that module is fully retired in a separate follow-up). ## 2. `AggIntent::Min` + `AggIntent::Max` → `QuantileApprox(Any)` Quantile sketches (DDSketch, KLL) answer min = quantile(0) and max = quantile(1) directly. Previously these returned `None` from `capability_for`; now they map to warm-tier-answerable. ## 3. Consolidate `SketchCapabilities` ↔ `SketchCapability` (Option A) Renamed `controller/src/sketch_algebra/schema.rs::SketchCapabilities` → `SketchStateMetadata` to remove the name collision with `capability.rs::SketchCapability`. Both files keep their roles: - `schema.rs::SketchStateMetadata` = L4 type-system role (wraps `SketchStateSchema` with sketch-family metadata per design.md §6.4). - `capability.rs::SketchCapability` = perf/feasibility/intent-routing metadata (consumed by optimizer + cost model). ## 4. Rename `params.rs` → `sketch_params.rs` Naming clarity. The file holds `SketchParams` + its KLL/DDSketch/Hll/ Cms/CountSketch variants. `sketch_algebra/mod.rs` retains `pub use sketch_params as params;` back-compat alias so the ~10 in-tree `crate::sketch_algebra::params::*` call sites keep compiling without further migration. ## 5. Frequency Capability surface: split FrequencyTopk vs FrequencyEstimate User-clarified MetricsQL surface mapping: | MetricsQL surface | AggIntent | Capability | |---|---|---| | `sum by (item) (rate(m[r]))` w/ Epsilon | `Frequency{accuracy}` | `FrequencyEstimate(Any)` | | `topk(k, sum by (item) (rate(m[r])))` | `TopK{k, accuracy}` | `FrequencyTopk(CmsWithHeap)` | | (Exact) | (any) | `None` — warm-tier doesn't carry exact | Concrete changes in `capability.rs`: - Added `Capability::FrequencyEstimate(SketchKindHandle)` (NEW — heap-LESS; answers bare per-key frequency without top-k extraction). - Existing `Capability::FrequencyTopk(SketchKindHandle)` stays (heap-BEARING; answers top-k). - Added `SketchKindHandle::CountSketchWithHeap` variant (alongside existing `CmsWithHeap`). - Updated `capability_for`: - `Frequency{!Exact}` → `Some(FrequencyEstimate(Any))` (was `FrequencyTopk(CmsWithHeap)` — wrong, that's for top-k only) - `TopK{Exact}` → `None` (was `FrequencyTopk` — exact top-k uses HashAgg+Heap, not a sketch) - Updated `is_satisfied_by`: - `FrequencyTopk(req)`: indexed must be `FrequencyTopk(have)` with `have ∈ {CmsWithHeap, CountSketchWithHeap}`. Rejects heap-less. - `FrequencyEstimate(req)`: indexed `FrequencyEstimate` with any frequency-family handle OR `FrequencyTopk` with heap-bearing handle (heap is additional info — underlying matrix answers the point query). ## Backend wire-in - `asap-query-engine/src/engines/warm_tier/sketch_reducer.rs`: new `QueryFamily::FrequencyEstimate` dispatch arm + heap-bearing variant routing. New `decode_frequency_total` helper emits per-window row-0 sum of the CMS/CountSketch matrix as the total-frequency scalar. - `asap-query-engine/src/drivers/ingest/otel.rs`: ingest-side capability mapping at sid registration time — heap-less sketches classify as `FrequencyEstimate`, heap-bearing as `FrequencyTopk`. ## Build + test - `cargo build --release -p controller` — clean - `cargo build --release -p query_engine_rust` — clean - `cargo test --release -p controller --lib` — **643/643 pass** (was 633 in PR #132; +10 new tests covering Min/Max, FrequencyEstimate semantics, is_satisfied_by wildcard + heap rules) - `cargo test --release -p query_engine_rust --lib -- engines::warm_tier` — **13/13 pass** (FrequencyEstimate ingest+reducer arm landed but no new fixture; the existing arms already exercise the Capability-keyed dispatch) ## Diff: 12 files, +546 / −140 ## Follow-ups (out of scope) - **FrequencyEstimate per-key lookup**: today's `decode_frequency_total` returns the row-0 sum (i.e., total frequency across the entire CMS matrix). A real per-key `frequency(metric, key)` reducer needs the key as an arg, which today's `function_args: &[f64]` signature can't carry. Documented inline. - **`AggIntent::HistogramQuantile` ghost in legacy_expr**: until legacy_expr is fully retired (PR #130 TODO 1), the variant lives on as a PromQL-operator placeholder. Removal happens with the legacy retirement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 11, 2026
Lowest-risk first batch of the legacy_expr retirement plan (Plan agent's
13-PR sequence). Deletes 5 QueryExpr variants + 5 ScalarExpr variants
that have ZERO constructor sites (only match arms), and renames
`QueryExpr::Dedup { col }` → `QueryExpr::Distinct { cols: Vec<ColumnRef> }`
with N-arity generalization.
## Deleted variants (zero constructor sites; only match arms)
QueryExpr family:
- `QueryExpr::JoinSketch { join_key, outer, inner }` — sketch-bound
physical alternative; design.md says L3 must be intent-only and the
sketch-bound shape belongs at L4 (`sketch_algebra::SketchExpr::SketchJoin`).
No constructor sites; 5 consumer match arms removed (query_parser,
physical/{stage_split,planner,allocator}, optimizer/engine).
- `QueryExpr::Subquery { alias, expr }` — designed for SQL nested
SELECT lowering, but no parser emits it. 6 consumer match arms.
- `QueryExpr::WindowFunc { func, partition_by, order_by, frame, input }`
— designed for SQL OVER() clauses, no parser emits it. 5 arms.
- `SketchCoverage` (legacy_expr-internal enum) — referenced only by an
in-file test. Deleted along with the test.
ScalarExpr family:
- `UnaryOp { op, input }` — 0 consumer arms.
- `InSubquery { expr, subquery, negated }` — 0 arms.
- `Case { operand, when_then, else_ }` — 0 arms.
- `Cast { expr, to }` — 0 arms.
- `VectorBinaryOp { op, lhs, rhs, vector_match }` — 0 arms (design.md
merges PromQL vector ops into `QueryExpr::BinaryOp { vector_match }`).
## Also dropped (orphaned after the D-purge)
- `UnaryOpKind` enum (only used by `UnaryOp`).
- `WindowFuncKind`, `WindowFrame`, `FrameUnit`, `FrameBound` (only used
by `WindowFunc`).
- Legacy `DataType` enum (only used by `Cast`). The canonical
`intent_algebra::schema::DataType` is untouched.
Reversible if Batch 2's canonical lift needs them.
## Dedup → Distinct rename + N-arity
`QueryExpr::Dedup { col: ColumnRef }` → `QueryExpr::Distinct { cols: Vec<ColumnRef> }`.
Per design.md §6 "What was removed" row 7: *"Single-column-only spelling
of SQL `DISTINCT`. Replacement: renamed to `Distinct { cols }`,
generalised to N columns"*.
Touched sites:
- legacy_expr.rs enum + walk/source_name match arms
- query_parser/mod.rs (sketch-mapping arm)
- physical/stage_split.rs (walk + PromQL serializer — 2 sites)
- physical/planner.rs (new `display_distinct_cols(&[ColumnRef])` helper)
- physical/allocator.rs (incl. routing doc table)
- optimizer/engine.rs (cost factor, deployment-stage map,
HLLDedupElim pattern, recursive rewriter, 2 tests)
- intent_algebra/legacy_lower.rs
HLLDedupElim rule struct name kept (semantically still describes the
rule); only test names/asserts updated.
## Canonical AggIntent verification
`controller/src/intent_algebra/agg_intent.rs` already has no
`HistogramQuantile` (removed in #133) and no `PromQLSubquery`. No
change needed.
## Build + test
- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **655 passed, 0 failed**
(was 656/656; -1 because `sketch_coverage_classification` test went
with the deleted `SketchCoverage` enum)
- `cargo test -p query_engine_rust --lib -- engines::warm_tier` — 13/13 pass
## Grep verification
- `JoinSketch | QueryExpr::Subquery | QueryExpr::WindowFunc | SketchCoverage |
ScalarExpr::{UnaryOp,InSubquery,Case,Cast,VectorBinaryOp}` → zero
matches across `controller/src/` and `asap-query-engine/src/`.
(The 6 remaining `::Subquery\b` hits are `Expr::Subquery(sq)` from
the external `promql_parser` / `sqlparser` AST libraries — not our
type; pre-existing and out of scope.)
- `QueryExpr::Dedup | Dedup {` → zero matches.
## Diff: 7 files, +66 / -323 (net -257 lines)
Co-authored-by: zz_y <zz_y@node0.zz-y-304941.softmeasure-pg0.clemson.cloudlab.us>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
zzylol
added a commit
that referenced
this pull request
May 12, 2026
…ubstitution (#144) Retires `legacy_expr::QueryExpr::HistogramQuantile` and optimizer rule R6 (`HistogramQuantileFusion`). `histogram_quantile(phi, m)` is now resolved at the parser site (`query_parser/promql.rs`) into a canonical `AggIntent::Quantile { q: phi }` — bucket-aware reduction is a physical- planner concern, not an L3 intent. Per the design invariant: `AggIntent::HistogramQuantile` does NOT exist in canonical L3 (PR #133 removed it). The optimizer's downstream fusion rule R6 was therefore vestigial — it matched on a wrapper that the parser no longer emits. Removing the rule + the legacy variant removes ~163 lines of dead surface across 17 files (optimizer/engine.rs -142 alone). Net effect: - Parser-side substitution maps `histogram_quantile(q, bucket_metric)` → `Quantile { q, accuracy }` at parse time. - `QueryShape::HistogramQuantile` in asap-query-engine's backend_storage_routing remains (it's a routing-table classifier, not an AggIntent — orthogonal concept). - Demo wires in `physical/{allocator, stage_split, planner}` and `sketch_algebra/{rules/bind_archive_only, tests}` simplified along with the rule retirement. cargo build --release -p controller clean; controller --lib 683/683. asap-query-engine builds clean (816 tests listable); local lib test run hangs on a futex unrelated to γ5 — CI will surface any γ5-attributable regression.
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
1 participant
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
User-flagged cleanup of
sketch_algebra/after PR #130's layered refactor.The 5 changes
1. Drop
AggIntent::HistogramQuantile✅histogram_quantile()is a PromQL/MetricsQL language-level operator, NOT a semantic intent. Removed the variant fromintent_algebra::agg_intent::AggIntent. The PromQL operator still lives on asintent_algebra::legacy_expr::QueryExpr::HistogramQuantile(unreachable from canonical L3 — never produced by the lowerer; PR #130 preserved legacy_expr for back-compat).2.
AggIntent::Min+AggIntent::Max→QuantileApprox(Any)✅Quantile sketches answer min = quantile(0) and max = quantile(1) directly. Previously returned
None; now warm-tier-answerable.3. Consolidate
SketchCapabilities↔SketchCapability(Option A) ✅Renamed
schema.rs::SketchCapabilities→SketchStateMetadatato remove the name collision withcapability.rs::SketchCapability. Both files keep their roles:schema.rs::SketchStateMetadata= L4 type-system role (wrapsSketchStateSchemaper design.md §6.4)capability.rs::SketchCapability= perf/feasibility/intent-routing metadata (consumed by optimizer + cost model)4. Rename
params.rs→sketch_params.rs✅Naming clarity.
sketch_algebra/mod.rsretainspub use sketch_params as params;back-compat alias so the ~10 in-tree call sites keep compiling without further migration.5. Frequency Capability surface:
FrequencyEstimatevsFrequencyTopk✅AggIntentCapabilitysum by (item) (rate(m[r]))w/ EpsilonFrequency{accuracy}FrequencyEstimate(Any)topk(k, sum by (item) (rate(m[r])))TopK{k, accuracy}FrequencyTopk(CmsWithHeap)NoneConcrete changes:
Capability::FrequencyEstimate(SketchKindHandle)(heap-LESS — answers bare per-key frequency)Capability::FrequencyTopk(SketchKindHandle)stays (heap-BEARING)SketchKindHandle::CountSketchWithHeap(alongside existingCmsWithHeap)capability_for(Frequency{!Exact})→Some(FrequencyEstimate(Any))(was wronglyFrequencyTopk(CmsWithHeap)— that's top-k only)capability_for(TopK{Exact})→None(exact top-k uses HashAgg+Heap, not a sketch)is_satisfied_by:FrequencyTopk(req): indexedhave ∈ {CmsWithHeap, CountSketchWithHeap}only — rejects heap-lessFrequencyEstimate(req): any frequency-family handle OR heap-bearing variants (heap is additional info)Backend wire-in
asap-query-engine/src/engines/warm_tier/sketch_reducer.rs: newQueryFamily::FrequencyEstimatedispatch arm;decode_frequency_totalemits per-window row-0 sum of the CMS/CountSketch matrix.asap-query-engine/src/drivers/ingest/otel.rs: ingest-side capability mapping at sid registration — heap-less →FrequencyEstimate, heap-bearing →FrequencyTopk.Build + test
cargo build --release -p controller— cleancargo build --release -p query_engine_rust— cleancargo test -p controller --lib— 656/656 pass (was 646 post-refactor(controller): trait impls + main.rs alias removal (PR #130 TODO 3+4) #132; +10 new tests covering Min/Max + FrequencyEstimate + is_satisfied_by wildcard + heap rules)cargo test -p query_engine_rust --lib -- engines::warm_tier— 13/13 passDiff: 12 files, +546 / −140
Follow-ups (out of scope)
decode_frequency_totalreturns the row-0 matrix sum. A real per-keyfrequency(metric, key)reducer needs the key as an arg, which today'sfunction_args: &[f64]signature can't carry. Documented inline.AggIntent::HistogramQuantileghost in legacy_expr: lives on as a PromQL-operator placeholder until legacy_expr is fully retired (PR refactor(controller): retire algebra/+planner/+config/+query_language/+analyzer.rs (Steps 2b/c/d/e/f) #130 TODO 1, currently blocked).🤖 Generated with Claude Code