Skip to content

feat(asap_types): make aggregationId optional; backend derives from content - #151

Merged
zzylol merged 1 commit into
mainfrom
feat/agg-id-optional
May 13, 2026
Merged

feat(asap_types): make aggregationId optional; backend derives from content#151
zzylol merged 1 commit into
mainfrom
feat/agg-id-optional

Conversation

@zzylol

@zzylol zzylol commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Phase 5 M2.1 — first half of the controller→backend aggregation_id handoff.
  • Adds compute_agg_config_id(metric, agg_type, sub_type, parameters, grouping_labels) -> u64 in crates/asap_types/src/aggregation_config.rs — xxh64 over a canonical encoding (parameters BTreeMap-sorted so HashMap insertion order doesn't matter).
  • Three deserializers (AggregationConfig::from_yaml_data, ::deserialize_from_json, StreamingConfig::from_yaml_data) now accept entries without aggregationId. When absent, the derived id fills in.
  • Backwards compatible: existing YAML with explicit aggregationId still parses and the value is honored unchanged.

What's next

  • M2.2 (next PR): drop aggregationId from controller/src/emit/asapquery_backend.rs output + retire deterministic_agg_id.
  • M2.3+ (future): migrate SchemaRegistry / SketchStore from agg_id-keyed to sid-keyed lookup (~362 references; deeper refactor).

Test plan

  • 5 new tests in aggregation_config::tests cover: explicit id honored, missing id derived deterministically, derivation sensitive to metric/parameters, derivation insensitive to HashMap insertion order.
  • 1 new test in streaming_config::tests proves a YAML without any aggregationId round-trips through from_yaml_data end-to-end.
  • cargo test -p asap_types --lib — 47/47 pass.
  • cargo check --workspace clean.

Note: capability_matching::tests::avg_finds_sum_and_count is a pre-existing flaky test (passes ~3/5 runs on main without these changes) due to non-deterministic HashMap iteration in find_compatible_aggregation. Not in scope for this PR.

🤖 Generated with Claude Code

@zzylol @claude
feat(asap_types): make aggregationId optional; backend derives from c…
d314642 
…ontent

Phase 5 M2.1 — first half of the controller→backend aggregation_id
handoff. The YAML deserializers (`AggregationConfig::from_yaml_data`,
`::deserialize_from_json`, `StreamingConfig::from_yaml_data`) now accept
entries without an `aggregationId` field. When absent, the backend
computes a deterministic u64 via `compute_agg_config_id(metric,
agg_type, sub_type, parameters, grouping_labels)` (xxh64 over a
canonical encoding — parameters are BTreeMap-sorted so insertion order
doesn't matter).

Backwards compatible: existing YAML that includes `aggregationId` still
parses and the explicit value is honored unchanged.

This is the consumer-side prep for M2.2, which will drop the field
from `controller::emit::asapquery_backend`'s output.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 955f1ab into main May 13, 2026
@zzylol zzylol mentioned this pull request May 13, 2026
Merged
5 tasks
zzylol added a commit that referenced this pull request May 13, 2026
@zzylol @claude
feat(controller): drop aggregationId from asapquery_backend YAML emis…
60f7149 
…sion (#152)

Phase 5 M2.2 — completes the controller→backend aggregation_id handoff.
`controller::emit::asapquery_backend::generate_streaming_config_yaml`
no longer writes the `aggregationId` field; the
`deterministic_agg_id` helper (and its dedicated test) is removed.

The backend's `AggregationConfig::from_yaml_data` now derives the id
deterministically via `compute_agg_config_id` (introduced in M2.1 /
PR #151) over the same fields the emitter writes, so the explicit
field was redundant.

In-tree YAML fixtures + the in-process feedback-loop test still mint
explicit ids; that path is exercised unchanged because explicit
`aggregationId` is honored for backwards compatibility.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request May 13, 2026
Merged
2 tasks
zzylol added a commit that referenced this pull request May 13, 2026
@zzylol @claude
feat(sketch_db): sid-level timeline_for_metric (#183)
8cd1476 
Schema retirement #1 of 5. Adds
`sketch_db::query::timeline::timeline_for_metric(&SketchStore, metric,
t1_ms, t2_ms) -> Vec<TimelineSegment>` that computes the per-metric
historical timeline entirely from the sid catalog
(`SketchStore::instances`).

Algorithm:

1. Snapshot all `SketchInstanceMetadata` for `metric` from the sid
   catalog.
2. Group by content signature `(metric, agg_kind, group_by_keys)`.
   Multiple sids sharing the same agg-config fold into one group.
3. Per group, fold the lifecycle fields: `min(first_seen_unix_ms)`,
   `Some(min(retired_at_ms))` iff every sid is retired,
   status = Active > Retired > Expired.
4. Apply the same segmenting + clipping as
   `SchemaRegistry::timeline_for_metric`.

`TimelineSegment.agg_id` now carries a stable xxh64 of the content
signature (the `(metric, agg_kind, group_by_keys)` tuple) — same
content-derived id idiom as `compute_agg_config_id` (PR #151). HTTP
callers see deterministic ids that don't depend on which specific
sid was first seen.

7 new unit tests cover: empty store, inverted range, single-active
signature, two-signatures-in-sequence, fold-of-many-sids-same-sig,
metric isolation, and signature-id determinism. Schema/'s
implementation stays alive in parallel until the consumers migrate.

Adds `SketchStore::snapshot_instances()` to support read-side scans.

783 + 7 = 790 lib tests pass.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol deleted the feat/agg-id-optional branch July 17, 2026 20:05
@zzylol zzylol mentioned this pull request Jul 22, 2026
Merged
4 tasks
zzylol added a commit that referenced this pull request Jul 22, 2026
@zzylol @claude
feat(control_plane): adopt asap_sketch::L4Node for the L4 IR (Step B) (…
c67075e 
…#408)

* feat(control_plane): adopt asap_sketch::L4Node for the L4 IR (Step B)

Retires control_plane's own locally-defined, flat PhysicalExpr L4 algebra
(Logical/SketchAgg/SketchEstimate/SketchMerge/ExactAgg) in favor of
ASAPController's canonical L4 IR, asap_sketch::{SummaryExpr, L4Node} --
the same move Step 3 of the enum-unification made for
SketchKind -> SummaryKind, one layer up.

PhysicalExpr is now a thin L5 wrapper: `Committed(L4Plan)` for the
common case, plus the two Phase eps.1 placement variants. L4Plan keeps
only what asap_sketch genuinely doesn't have -- named LetBinding/Ref
fan-in sharing (asap_sketch's own DAG sharing is structural, via Rc, but
this crate's rule-firing walk still needs a name to thread a bound value
across sibling calls).

The 7 bind_kll_quantile/bind_ddsketch_quantile/bind_hll_cardinality/
bind_cms_count/bind_cms_topk/bind_exact_agg/bind_archive_only Rule
structs are retired -- their selection/sizing policy (KLL's k rungs,
DDSketch-over-KLL priority, TopK recall tiers, wire-cost tie-breaks) is
preserved verbatim in the new ControlPlaneCostModel, plugged into
asap_plan::bind::implement_tree_in_with via the CostModel trait
(rank_candidates + the new size_params hook, ASAPController#146) instead
of a bespoke dispatcher -- so schema derivation, col/by computation, and
DAG construction are asap_plan::bind's, not a forked copy.

Three node shapes get a small local pre-pass in sketch_algebra::lower
before delegating, because asap_plan::boundary::implementation_for
actively binds them to something this deployment's data plane can't (or
deliberately shouldn't) serve:
- AggIntent::Count{accuracy: Exact} would bind SummaryKind::Count, which
  has no data-plane accumulator (PR #200/#201 already established this
  is wrong -- reverted, stays on archive).
- AggIntent::Rate would bind its own SummaryKind::Rate; this deployment
  represents Rate as an Increase accumulator (rate = increase / window,
  a query-time division, not a separate accumulator).
- AggIntent::Extension (Frequency) and TopK{accuracy: Exact} both
  decline to bind at all (asap_plan's Extension/exact-TopK coverage gaps
  -- filed upstream as ASAPController#150 and #151); this is a real,
  accepted behavior change from the retired bind_cms_count/bind_cms_topk
  rules, not a bug -- see the updated tests in optimizer/rules/mod.rs
  and emit/mod.rs.

Also fixes a real bug surfaced by the migration: PhysicalExpr now
carries Rc<L4Node> (asap_sketch's own DAG-sharing mechanism), so holding
a bound PhysicalExpr across an .await point made handle_plan's generated
Future !Send, breaking axum::Handler. Scoped the Rc-bearing computation
into a synchronous block that resolves down to Send-safe StageConfig
output before the first await.

Pins ASAPQuery-backend's ASAPController dependency to
12482fd77945ab5771a021c6112750fd9284f8ed (ASAPController PR #146,
CostModel::size_params + bind::logical).

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

* chore: re-pin ASAPController to main (post #146/#154 merge)

ASAPController#146 (CostModel::size_params) and #154 (bind::logical
visibility) are both merged. Re-pin from the feat/costmodel-size-params-hook
branch tip to main's current commit now that both land there.

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

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request Jul 29, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@zzylol