feat(controller): Phase B — intent_algebra L3 IR (AggIntent + QueryExpr + Schema flow) - #275
Merged
Merged
Conversation
…pr + Schema flow) Per design.md §6 "core::intent_algebra — Layer 3", introduce the L3 intent IR the planner pivots on, in a new `controller/src/intent_algebra/` module. Phase A (#273) shipped the typed QuerySpec types; this PR ships the algebra + schema flow they describe a workload over. What lands: - `AggIntent` — vocabulary of "what to compute, not how": Count, Sum, Min, Max, Avg, Quantile{q, accuracy}, TopK{k, accuracy}, Cardinality{accuracy}, Frequency{accuracy}, Rate{window}, Increase{window}. No sketch types here — sketch binding is L4. - `QueryExpr` — the L3 algebra DAG, single-rooted per query: Scan, Window, Aggregate, LetBinding, Ref. Variant subset chosen for the DC + PromQL deployment scope per the orchestrator spec; Filter/Project/ Partition/Distinct/Merge/Join/SetOp/Sort/Limit/Subquery/WindowFunc/ BinaryOp are deferred so each lands with a planner consumer rather than as dead code (additive growth). - `Schema` — typed schema flowing on every L3 edge with `unique_keys` populated per design.md §6 schema-flow table. `unique_keys` is the load-bearing CSE-legality field (design.md §6 line ~1284 + the batched-queries example); `cse_substitution_legal_only_with_unique_keys` pins this invariant as a unit test. - `lower_parsed_query(parsed, accuracy) → QueryExpr` — single-query lowering from the existing `query_parser::ParsedQuery`. Workload- level CSE that produces fan-in (multi-root with LetBinding/Ref) is the follow-up's job. Per-variant schema propagation rules match the design.md table: Scan emits the source schema with unique_keys from its catalog; Window propagates row identity (carries unique_keys verbatim, requires time_index on input); Aggregate{by, ..} emits unique_keys = [by] and strips the time axis; LetBinding/Ref propagate the bound expr's schema. Wire-up state. Nothing in `analyzer::Analyzer` or `planner/` consumes these types yet — that's the follow-up Phase C PR. Phase B exposes the IR so that wiring becomes a focused change rather than co-emission of new types + new consumers. Tests added (in module): agg_intent_serde_roundtrip, output_column_names_are_intent_keyed, quantile_output_is_float64, sum_preserves_input_dtype, schema_serde_roundtrip, schema_new_*, schema_with_time_index_*, add_unique_key_dedupes, query_expr_simple_aggregate, query_expr_let_binding_ref, query_expr_unresolved_ref_errors, query_expr_window_requires_time_index, query_expr_aggregate_invalid_by_column, query_expr_serde_roundtrip, cse_substitution_legal_only_with_unique_keys, lower_promql_basic, lower_promql_with_group_by, lower_promql_cardinality, lower_empty_metric_errors. design.md §6 grows an "Implementation status" subsection naming the Phase B subset and the deferred phases C/D/E/F. Note on cargo test status. The controller's `cargo test` target was already broken at origin/main (post-#273): nine `data_sink` field-init errors in `src/config/agent.rs`, `src/config/asapquery_backend.rs`, `src/config/precompute.rs`, and `src/main.rs` test code. This is pre-existing breakage from the `data_sink` field added to `AgentCollectorConfig` in #204; intent_algebra introduces no new test errors. `cargo build` and `cargo clippy --bins` are both clean for intent_algebra. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # controller/src/main.rs
5 tasks
zzylol
added a commit
that referenced
this pull request
May 6, 2026
…nfig constructors (#276) PR #204 added the `data_sink: AgentDataSink` field to `AgentCollectorConfig` (production sites updated) but missed nine test-fixture / test-code constructors, leaving `cargo test --release -p controller` blocked at compile-time. This broke the baseline for downstream PRs (#273, #274, #275) which could not run their newly added unit tests. Mechanical fill-in only, no semantic change to existing tests: - 7 sites use `AgentDataSink::default()` (Otlp-to-backend) — the canonical default that PR #204 introduced for new pipelines. - 2 sites (`config::agent::tests::ddsketch_cfg` and `main::api_tests::generated_agent_yaml_contains_opamp_extension`) pin `AgentDataSink::PrometheusScrape { endpoint: "0.0.0.0:8889" }` because their pre-existing assertions check for the legacy `prometheus` exporter on :8889. Pinning the sink keeps the test semantics intact rather than rewriting the asserts. After the fix: - `cargo build --release -p controller` clean. - `cargo test --release -p controller --no-run` clean (was the blocker). - `cargo test --release -p controller` runs 395 tests; 389 pass, 6 pre-existing failures unrelated to `data_sink`: * 2 in `analyzer::tests` — float-precision asserts. * 4 in `opamp::tests` / `api_tests` — protobuf framing ("invalid tag value: 0") on `ServerToAgent` decode. These are tracked separately and out of scope for this PR. - Newly-shipped tests now run end-to-end: `query_language` (8), `language_logical_plan` (6), `types_v2` (7), `algebra` (intent_algebra family, 6+). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 6, 2026
…eys CSE legality (#277) Per design.md §6 `core::cost` (line ~997) and Schema flow (line ~425), land the workload-level cost-model entry point + the load-bearing consumer of `Schema::unique_keys`. Phase A/B/D shipped the typed types, the L3 IR DAG with `Schema::unique_keys`, and the language wrap; Phase F is what makes `unique_keys` "load-bearing" and what credits shared sub-DAGs in the bundled cost. What lands: - `planner::cost_model::workload_cost(plan: &WorkloadCostPlan<'_>) -> Result<WorkloadCost, QueryExprError>` — walks the L3 IR DAG (Phase B's `intent_algebra::QueryExpr`) post-order, memoises by `LetBinding` name, credits each shared producer once across consumers. Returns `WorkloadCost { total_dollars, per_root_breakdown, reused_savings }`. `reused_savings` exposes the gap between the bundled total and the naive sum-over-roots, for EXPLAIN / observability per design.md §6 line ~1023. - `planner::cost_model::WorkloadCostPlan { bindings, roots }` — the cost-model's view of `types_v2::WorkloadPlan` carrying real `&QueryExpr` references rather than the `QueryExprPlaceholder` JSON- wire string. Collapses into `types_v2::WorkloadPlan` when the placeholder is swapped for live `QueryExpr` downstream. - `intent_algebra::schema::cse_reuse_is_legal(producer_schema, consumer_count) -> Result<(), CseError>` — the gatekeeper. Two `QueryExpr::Ref` consumers may share a producer only when the producer's output schema has at least one `unique_keys` set and the consumer count is ≥ 2. This is the proof point that `Schema::unique_keys` is load-bearing — without it the deduper conservatively refuses to share and reuse "drops on the floor" (design.md §6 line ~1356). - `intent_algebra::cse::dedupe_subtrees(roots) -> CseWorkloadPlan` — basic implementation of the workload-level CSE pass. Detects shared `Aggregate` children across ≥2 roots, gates on `cse_reuse_is_legal`, hoists into a `LetBinding`. The full alpha-equivalence + nested-CSE algorithm is downstream — Phase F lands the gate + the basic case so the cost-model side has something to credit. Per-node cost primitives at L3 (`node_cost_scan`, `node_cost_window`, `node_cost_aggregate`, `intent_cost`) are coarse-but-monotonic placeholders calibrated against schema width and intent kind. What Phase F pins is the *shape* (positive, additive, savings invariant `bundled_total ≤ naive_sum`); calibration against real benchmarks is downstream. Tests added (15): - `planner::cost_model::workload_cost_tests` (7): - `workload_cost_single_root_equals_query_cost` — degenerate case - `workload_cost_two_roots_no_sharing_equals_sum` — independent queries - `workload_cost_two_roots_shared_window_credits_once` — design.md batched-queries example: 2 quantile queries share Window+Scan; `reused_savings ≈ shared_cost` - `workload_cost_three_roots_two_share_partial` — q1+q2 share, q3 independent; savings = 1× shared_cost - `workload_cost_three_roots_all_share_one_binding` — three consumers, savings = 2× shared_cost - `workload_cost_unused_binding_is_zero_savings_not_negative` — defensive non-negative invariant - `workload_cost_unresolved_ref_errors` — `Ref` to undeclared name surfaces as `UnresolvedRef` - `intent_algebra::schema::tests` (4 added — total 8 with the 4 pre-existing schema tests): - `cse_reuse_legal_when_unique_keys_set` — green path - `cse_reuse_illegal_when_unique_keys_empty` — refused without unique_keys - `cse_reuse_rejects_single_consumer` — short-circuit for count < 2 - `cse_reuse_consumer_check_precedes_unique_key_check` — error- ordering invariant - `intent_algebra::cse::tests` (4): - `dedupe_subtrees_empty_input` - `dedupe_subtrees_single_root_passthrough` - `dedupe_subtrees_basic` — design.md batched-queries example: two queries with identical `Window` sub-trees get hoisted - `dedupe_subtrees_no_shared_subexpr` — no fan-in detected → no binding emitted design.md grows two "Implementation status" subsections — one under `core::cost` describing what `workload_cost` ships and what's deferred (per-plan latency split, `ReusedComponent` enumeration), and one under the Schema flow table describing `cse_reuse_is_legal` + the basic `dedupe_subtrees` shape. Note on cargo test status. The controller's `cargo test` target was already broken at origin/main (post-#275): nine `data_sink` field-init errors in `src/config/agent.rs`, `src/config/asapquery_backend.rs`, `src/config/precompute.rs`, and `src/main.rs` test code. This is the pre-existing baseline blocker called out in the orchestrator spec (separate fix in flight, #28). `cargo build --release -p controller` and `cargo clippy --release --bin controller` both come out at the same warning / error count as `origin/main` (132 build warnings, 162 clippy errors — all pre-existing in unmodified files). Phase F adds zero new warnings or clippy errors. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 17, 2026
… renames metric names (#382) Closes the audit thread on "do asapcollector processors add a metric- name suffix on the wire?" The runtime answer is no — the 2026-05 refactor moved sketch-type identification from name suffixes to the OTLP `pdata.Metric` variant tag (DDSketch / KLLSketch / HLLSketch / CountSketch / CountMinSketch), and every production-path encode site now preserves the input metric name: * DDSketch (`shim_helpers.go::appendSketchMetrics`): `env.MetricName = inputName` * KLL transmit_sketch=true (`encode.go::sketchMetricName`): returns base * HLL (`encode.go::cardinalityMetricName`): returns base * CountSketch / CountMin: no MetricSuffix field at all But the dead `MetricSuffix string` config field plus the configuration plumbing left behind made the code look like a suffix SHOULD be applied. This PR removes the dead config field across DDSketch / KLL / HLL, deletes the only remaining application site (the KLL fallback `transmit_sketch=false` quantile-CDF path that production never enables), and strips `metric_suffix:` lines from every agent + gateway YAML in `deploy/mvp-{singlenode,multinode}/`. Code touched: * `ddsketchprocessor/{config,shim_helpers,processor_test}.go` — field + 12 test references gone * `kllprocessor/{config,factory,encode,processor_test}.go` — field + fallback application + test references gone * `hllprocessor/{config,factory,processor_test}.go` — field + dedicated TestBatchModeMetricSuffix test gone * Stale doc table in `processor/ddsketchprocessor/README.md` * 13 agent yamls + 2 gateway yamls: `metric_suffix:` lines stripped * README + inline-comment updates calling out the refactor Build verification: * `bash build_asap_otel.sh --skip-patches` — full OCB build of the asap-otel binary succeeds with the patched processors compiled in (binary written to opentelemetry-collector-contrib-patch/cmd/asap-otel/asap-otel) * `go build ./...` per-processor passes * `go test ./...` per-processor has pre-existing failures from the SAME 2026-05 proto refactor (tests still reference removed wire fields `dp.Count`, `dp.Cardinality`, `dp.Precision` etc) — verified unchanged on clean main; out of scope for this PR. A few yaml comment lines still mention `metric_suffix` historically (README-allsketches-demo.md, backend-inference-kll.yaml, asap-otel-agent-allsketches.yaml, backend-inference.yaml) — they're inline comments documenting the previous behavior, harmless at runtime. Companion to ASAPQuery-backend #275 (`revert+retire(query): delete dead resolve_sketch_metric_alias rewrite`) which removed the backend-side counterpart of this same retired suffix scheme. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
Phase B of the controller-design alignment per
design.md§6core::intent_algebra— Layer 3. Phase A (#273) shipped the typed QuerySpec types (QueryShape/DataShape/AccuracyTarget/QueryLanguage); this PR ships the L3 algebra + schema flow they describe a workload over.AggIntent— vocabulary the planner pivots on (Count, Sum, Min, Max, Avg, Quantile{q, accuracy}, TopK{k, accuracy}, Cardinality, Frequency, Rate, Increase). No sketch types here — sketch binding is L4.QueryExpr— single-rooted L3 algebra DAG:Scan,Window,Aggregate,LetBinding,Ref. Variant subset chosen for the DC + PromQL deployment scope; the remaining design.md variants are deferred so each lands with a planner consumer rather than as dead code (purely additive growth).Schema— typed schema on every L3 edge, withunique_keyspopulated per the design.md §6 schema-flow table.unique_keysis the load-bearing CSE-legality field (design.md §6 line ~1284 + the batched-queries example); a unit test pins the invariant.lower_parsed_query(parsed, accuracy) -> QueryExpr— single-query lowering from the existingquery_parser::ParsedQuery. Workload-level CSE that produces multi-root fan-in is the follow-up.Schema::unique_keyspropagation per variant:Scan[ts, *labels])Windowtime_indexon inputAggregate { by, .. }[by](the group-by tuple is unique by construction); stripstime_indexLetBindingchild's output schemaRefdesign.md §6 grows an "Implementation status" subsection naming this PR's subset and the follow-up phases C/D/E/F.
Files
controller/src/intent_algebra/{mod,agg_intent,query_expr,schema,lower}.rs(new)controller/src/main.rs—mod intent_algebra;declaration (single-line addition)controller/docs/design.md— Implementation-status subsection in §6Wire-up state
Nothing in
analyzer::Analyzerorplanner/consumes these types yet. Phase C will wireAnalyzer::analyzeto also produce aQueryExpralongsideQueryWorkload; Phase D will add the workload-level CSE pass that populatesWorkloadPlan::bindings.Test plan
cargo build --release -p controller— clean (133 warnings, same as baseline; 0 from intent_algebra)cargo clippy --release -p controller --all-targets— 0 issues from intent_algebracargo test --release -p controller intent_algebra— blocked by pre-existing baseline breakage: 9data_sinkfield-init errors insrc/config/agent.rs,src/config/asapquery_backend.rs,src/config/precompute.rs, andsrc/main.rstest code. Thedata_sinkfield was added toAgentCollectorConfigin feat(e2e): all-five-sketch runtime path + harness — controller, processor, fake-exporter, P1–P9 #204 without updating test setups, and the controller's test target hasn't compiled at HEAD since. intent_algebra introduces no new test errors; fixing the baseline is out of Phase B's file domain (controller/src/types.rs,src/config/, andsrc/main.rstest code are off-limits per the spec).19 in-module tests written, naming the invariants they pin:
agg_intent_serde_roundtrip,output_column_names_are_intent_keyed,quantile_output_is_float64,sum_preserves_input_dtype,schema_serde_roundtrip,schema_new_has_no_time_or_unique_key,schema_with_time_index_populates_metadata,add_unique_key_dedupes,query_expr_simple_aggregate,query_expr_let_binding_ref,query_expr_unresolved_ref_errors,query_expr_window_requires_time_index,query_expr_aggregate_invalid_by_column,query_expr_serde_roundtrip,cse_substitution_legal_only_with_unique_keys,lower_promql_basic,lower_promql_with_group_by,lower_promql_cardinality,lower_empty_metric_errors.🤖 Generated with Claude Code