Skip to content

feat(core): interface types for L1 input, L3 intent algebra, L4 sketch algebra - #3

Merged
milindsrivastava1997 merged 2 commits into
mainfrom
dev-milind
May 16, 2026
Merged

milindsrivastava1997 merged 2 commits into
mainfrom
dev-milind

Conversation

@milindsrivastava1997

Copy link
Copy Markdown
Collaborator

Summary

  • Adds Cargo workspace skeleton (Cargo.toml at root, crates/core/)
  • Defines L1 input boundary: QueryWorkload, QueryLanguage (with SqlDialect variants), BatchEntry, RepeatingEntry, DataCharacteristics, DataDistribution, QueryRequirements
  • Defines L3 output boundary (intent_algebra): QueryExpr DAG with Rc<L3Node> children, L3Schema/L3DataType/L3Field, HasSchema trait, AggIntent, Source, TimeWindowKind (renamed from WindowKind)
  • Defines L4 output boundary (sketch_algebra): SummaryExpr DAG with Rc<L4Node> children, L4Schema/L4DataType (Primitive(L3DataType) | Sketch(SketchKind, SketchParams)), SketchKind, SketchParams, SketchQuery
  • Shared AccuracyTarget enum (Epsilon / EpsilonDelta / Exact) in types.rs

Design choices

  • Node wraps schema: L3Node { expr: QueryExpr, schema: L3Schema } and L4Node { expr: SummaryExpr, schema: L4Schema } — every Rc<*Node> pointer IS an edge, carrying the child's output schema inline. No separate Edge type.
  • L3/L4 schemas are separate types: L4DataType::Primitive(L3DataType) wraps L3 types; L4DataType::Sketch(...) is L4-only. L3 nodes structurally cannot carry sketch-typed columns.
  • No logic: all methods on AggIntent and HasSchema are todo!() stubs. Zero external dependencies.

Test plan

  • cargo check passes with zero warnings

🤖 Generated with Claude Code

…L4 sketch algebra

Introduces the Cargo workspace skeleton and the `asap-control-core` crate
with pure interface/boundary types for three pipeline layers — no logic,
no I/O, no external dependencies.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@milindsrivastava1997

Copy link
Copy Markdown
Collaborator Author

Exact accumulators need to be representable in SketchKind

In ASAP, not all precomputed aggregations are approximate. Sum, Count, Min/Max, Increase, and Rate are stored and merged in the sketch store with exact semantics — they map to AggregationType::{Sum, MinMax, Increase, ...} in the current asap-query-engine.

If these stay as Logical(Aggregate) nodes in L4, ASAPController can't emit SummaryRead for them, and asap-query-engine would fall through to a DataFusion execution path that expects raw data rather than precomputed accumulators.

Proposed fix: extend SketchKind with exact-accumulator variants:

pub enum SketchKind {
    // Exact accumulators (no approximation error)
    Sum,
    Count,
    MinMax,
    Increase,
    Rate,
    // Approximate sketches (existing)
    Kll,
    Cms,
    Hll,
    DDSketch,
    CmsWithHeap,
    Kmv,
    Theta,
}

This keeps the L4 representation uniform — all precomputed aggregations (exact or approximate) flow through the same SummaryRead → SummaryMerge → SummaryEstimate pipeline. The naming is a slight misnomer for the exact variants, but the alternative (a parallel AccumulatorKind + SummaryAccumulate node) doubles the node types and translation logic downstream with no real benefit at this stage.

Also needed in the same PR: a SummaryRead node added to SummaryExpr (distinct from SummaryAgg). SummaryAgg implies computing a sketch from raw data; SummaryRead means fetching a precomputed sketch from the ASAP store. Proposed shape:

SummaryRead {
    metric: MetricRef,
    sketch: SketchKind,
    params: SketchParams,
    by: Vec<GroupKey>,
    time_window: TimeWindowKind,
    time_range: TimeRange,   // relative Duration for repeating queries, absolute for batch
    spatial_filter: Option<LabelFilter>,
}

@milindsrivastava1997

Copy link
Copy Markdown
Collaborator Author

One clarification on SummaryRead: it doesn't strictly need to live in asap-control-core. Since SummaryRead is specific to ASAP's precomputed-sketch-store model (reading a stored aggregation by agg_id, time range, and sketch type), it can be defined in a downstream artifact — either asap-query-engine itself or a shared ASAP-specific crate (e.g. ASAPCollector / asap-common). That keeps asap-control-core's L4 IR general-purpose and free of storage-model assumptions. The asap-control-core PR only needs the SketchKind extension (D5 above); SummaryRead is a downstream concern.

@milindsrivastava1997
milindsrivastava1997 merged commit b80b951 into main May 16, 2026
@milindsrivastava1997
milindsrivastava1997 deleted the dev-milind branch May 16, 2026 19:18
zzylol added a commit that referenced this pull request May 27, 2026
…#2, #3)

Locks behavior introduced by the recent fixes, +5 tests (125→130):

- promql_conformance: nested unary negation propagates rejection
  (`a - -b`, `sum(-x)`), not just top-level; and count→Cardinality threads
  the AccuracyTarget (Exact stays exact, Epsilon carried) — pins review #2.
- sql_lowering: a qualified WHERE on the *duplicated* join column
  (`WHERE hosts.service = ...`) binds to the qualified position (4), not the
  first `service` (1) — extends the #7 disambiguation past the join key.
- error.rs: `UnsupportedFeature` Display is language-neutral (no "PromQL") —
  pins the #3 fix without depending on DataFusion plan shapes.
- schema.rs: `Column.table` deserializes to `None` when the key is absent
  (the `#[serde(default)]` backward-compat contract), and a qualified column
  round-trips.

Full workspace green (130 tests), clippy -D warnings + fmt clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 27, 2026
… mark reserved nodes (L2 review #3/#4/#6)

- #4: remove `AggItem.distinct` — a write-only field the converter never read,
  redundant with `AggFunc::CountDistinct` (COUNT(DISTINCT) sets the func; value
  DISTINCT is rejected up front). It encoded "distinct" a second way, consumed
  zero ways.
- #6: `AggItem.alias` is now `Option<String>` (was a `""` sentinel), matching
  its sibling `L2ProjectItem.alias`. The converter maps `None → ""` for L3's
  `output_names` sentinel; PromQL emits `None`, SQL `Some(name)`.
- #3: doc-mark the L2 nodes no front end produces as **Reserved** —
  `Ref`/`LetBinding` (CSE is L3), `Merge` (UNION→SetOp), L2 `Partition` (PromQL
  emits `Aggregate.keys`), and `PartitionKeys::Without` (rejected up front) — so
  the dead converter arms read as intentional, not oversights.

No behavior change. Full suite green (133), clippy + fmt clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 24, 2026
Restructure the flat 19-section developer guide into the three-part
structure the doc owner asked for: Part 1 - Code Architecture, Part 2
- Interfaces and Definitions, Part 3 - How to Add X, Y, Z (each ending
in how to verify). Content is moved, not rewritten:

Part 1 (Mental model first, per doc-owner follow-up, then a new
whole-PR architecture diagram, then "How the current pieces fit
together"):
- old #1 Mental model -> Part 1 #1
- new: whole-PR architecture diagram (TargetSubDAG's two entry points
  through ReplacementStrategy, PlanSpace/cost_sorted, explanation.rs,
  to a downstream consumer) -> Part 1 #2
- old #3 How the current pieces fit together -> Part 1 #3

Part 2:
- old Terminology's "Implementation" definition merged into the
  Glossary as one more entry (### Implementation), next to
  ReplacementStrategy
- old #2 Glossary -> Part 2 #1 (plus the merged Implementation entry
  and old #10 Matcher, retitled to match glossary-entry style)
- old #10 Matcher (implementation.rs) -> ### Matcher inside the
  Glossary; implementation.rs no longer exists, so the stale title
  is fixed
- old #19's definitional content (ReplacementExplanation/
  ExplanationKind shapes, node_hash, why there's no ExplanationRule
  trait, location-text ownership) -> Part 2 #2

Part 3:
- old #4, #5, #6, #7, #13, #14 -> Part 3 #1, Adding a new
  ReplacementStrategy (ending in Testing a new strategy)
- old #8, #9, #15 -> Part 3 #2, Adding or customizing a CostModel
  (ending in Testing a new cost model)
- old #12 -> Part 3 #3, Adding a new sketch algorithm, with its
  stale implementation.rs/binder references fixed to replacement.rs/
  construct_summary vocabulary, plus a new "Verifying a new sketch
  algorithm" close grounded in the existing coverage-matrix tests
- old #11, #16, #17, #18 -> Part 3 #4-#7 (capstone + closing
  reference material); #18's extension-map table's implementation.rs
  row fixed to replacement.rs
- old #19's "Using it"/"Adding a new kind" content -> Part 3 #8,
  Using and extending explanation.rs

cargo build --workspace --all-targets is clean (docs-only change).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant