Skip to content

Step 5: split AggregationType into AccumulatorSpec (SummaryKind + typed params + grouping) - #401

Merged
zzylol merged 3 commits into
mainfrom
feat/accumulator-spec-split
Jul 22, 2026
Merged

zzylol merged 3 commits into
mainfrom
feat/accumulator-spec-split

Conversation

@zzylol

@zzylol zzylol commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Step 5 of the sketch-identity unification (scratchpad/artifacts/enum-unification-plan.md, §6/§8) — the highest-risk step, landed as its own PR per the plan's sequencing.

data_plane's AggregationConfig represented "which accumulator to run, on what, with what parameters" as three loosely-typed things: a 16-variant AggregationType enum that conflates sketch identity with the keyed/unkeyed axis, a raw aggregation_sub_type: String consulted only for two "wrapper" variants (SingleSubpopulation/MultipleSubpopulation), and an untyped parameters: HashMap<String, Value> bag. The real dispatch (accumulator_factory.rs::create_accumulator_updater) was a 14-arm match with a second string-typed dispatch layer underneath it (~54 AggregationType references in that one file).

This adds asap_types::AccumulatorSpec { kind: SummaryKind, params: SummaryParams, grouping: Option<KeyByLabelNames> }, converging data_plane onto the same asap_sketch::SummaryKind/SummaryParams representation control_plane already uses (Stage 3, merged — same asap-sketch git pin, rev = 150ef7d...), extended with the keyed/unkeyed grouping axis AggregationType wrongly folded into identity. accumulator_factory.rs now dispatches on AccumulatorSpec.kind instead of the AggregationType + string combo; the wrapper string-matching arms collapse away now that grouping is a sibling field. Every one of the old 14 match arms' behavior is preserved exactly — including the CMS-heap-vs-bare distinction, the bare-CountSketch-shares-CmsAccumulatorUpdater quirk, and all three fallback/unknown-warning paths (same warning text, same default updater per path).

Two decisions flagged for review

1. Additive, not a full replacement. AggregationConfig keeps its aggregation_type/aggregation_sub_type/parameters fields untouched — AccumulatorSpec is computed on demand via AggregationConfig::accumulator_spec().

  • PolicyFingerprint::from_config hashes those three fields directly, and its own module doc calls the byte layout it produces a stability contract ("Don't reorder fields... any such change invalidates every deployed fingerprint and forces a cold-start rebuild"). policy_fingerprint.rs is not touched at all by this PR.
  • Separately: AggregationType turned out to be read by ~40 files across data_plane/asap_types (query-time capability matching in capability_matching.rs, persistence/sid_metadata.json round-trip, reconciliation, index maintenance) — well beyond accumulator_factory.rs. Full removal was judged too large to land and review safely in one PR; tracked as follow-up.

2. Three details don't fit asap_sketch's upstream types, documented in accumulator_spec.rs's module doc, and still read AggregationConfig/its parameters map directly from accumulator_factory.rs:

  • Min/max direction — SummaryParams::MinMax carries no fields.
  • HydraKLL's (row, col) tiling grid — SummaryParams::Kll carries only k.
  • Top-k weight_mode — no upstream concept at all.

The wire format (aggregationType/aggregationSubType/parameters JSON and YAML keys) is unaffected — AggregationConfig::from_yaml/from_json still parse those key names generically off a serde_yaml::Value/serde_json::Value, unchanged.

Test plan

  • cargo build --workspace — clean
  • cargo test -p asap_types — 78 passed (18 new: every AggregationType variant's resolution, both wrapper sub_type alias lists, all three error paths, a PolicyFingerprint stability regression guard)
  • cargo test -p data_plane --lib — 881 passed, 2 ignored, 0 failed — matches pre-existing baseline exactly, no regressions
  • cargo clippy -p asap_types -p data_plane --lib — no new warnings
  • cargo fmt on touched files only — verified via git status that no incidental reformatting of untouched files leaked into the diff

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the feat/accumulator-spec-split branch 2 times, most recently from c635ae8 to 32bb4cc Compare July 21, 2026 16:02
zzylol and others added 3 commits July 22, 2026 10:48
Step 5 of the sketch-identity unification (scratchpad/artifacts/enum-
unification-plan.md, section 6/8). data_plane's AggregationConfig has
represented "which accumulator to run, on what, with what parameters"
as three loosely-typed things: a 16-variant AggregationType enum that
conflates sketch identity with the keyed/unkeyed axis, a raw
aggregation_sub_type string consulted only for two "wrapper" variants,
and an untyped parameters: HashMap<String, Value> bag. The real
dispatch (accumulator_factory.rs::create_accumulator_updater) was a
14-arm match with a second string-typed dispatch layer underneath it.

This adds asap_types::AccumulatorSpec { kind: SummaryKind, params:
SummaryParams, grouping: Option<KeyByLabelNames> }, converging
data_plane onto the same asap_sketch::SummaryKind/SummaryParams
representation control_plane already uses (Stage 3, merged), extended
with the keyed/unkeyed grouping axis AggregationType wrongly folded
into identity. accumulator_factory.rs now dispatches on
AccumulatorSpec.kind instead of the AggregationType + string combo;
the SingleSubpopulation/MultipleSubpopulation string-matching arms
collapse away now that grouping is a sibling field. Every one of the
old 14 match arms' behavior is preserved exactly, including the CMS-
heap vs bare-CMS distinction, the bare-CountSketch-shares-CmsAccumulator
quirk, and all three fallback/unknown-warning paths (same warning
text, same default updater per path).

Two decisions worth flagging for review:

- Additive, not a replacement. AggregationConfig keeps its
  aggregation_type/aggregation_sub_type/parameters fields untouched.
  PolicyFingerprint::from_config hashes those three fields directly
  and its own module doc calls the byte layout it produces a stability
  contract ("any such change invalidates every deployed fingerprint
  and forces a cold-start rebuild") -- so policy_fingerprint.rs is not
  touched by this change at all. Separately, AggregationType turned
  out to be read by ~40 files across data_plane/asap_types (query-time
  capability matching, persistence, reconciliation, index maintenance)
  well beyond accumulator_factory.rs, so full removal was judged too
  large to land and review safely in one PR. AccumulatorSpec is
  computed on demand from AggregationConfig's existing fields via
  AggregationConfig::accumulator_spec(); full removal of the old
  fields is follow-up work, not done here.
- Three details don't fit asap_sketch's upstream types and still read
  AggregationConfig/its parameters map directly, documented in
  accumulator_spec.rs's module doc: min/max direction (SummaryParams::
  MinMax carries no fields), HydraKLL's (row, col) tiling grid
  (SummaryParams::Kll carries only k), and top-k weight_mode (no
  upstream concept at all).

The wire format (aggregationType/aggregationSubType/parameters JSON
and YAML keys) is unaffected -- AggregationConfig::from_yaml/from_json
still parse those key names generically, unchanged.

cargo test -p asap_types: 78 passed (18 new, covering every
AggregationType variant's resolution, both wrapper sub_type alias
lists, all three error paths, and a fingerprint-stability regression
guard). cargo test -p data_plane --lib: 881 passed, 2 ignored -- no
change from the pre-existing baseline. cargo build --workspace: clean.

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

promql_utilities was deleted in #403 (merged to main after this branch
was cut); AggregationType now lives in-crate at
asap_types::aggregation_type. Update the one remaining import site
picked up by the rebase.

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

Picked up by rebasing onto main post-#405 (WindowType retired in
favor of asap_ir::WindowKind). Test-only fixture reference, no
behavior change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/accumulator-spec-split branch from 32bb4cc to c58239a Compare July 22, 2026 16:51
@zzylol
zzylol merged commit e529e7b into main Jul 22, 2026
@zzylol
zzylol deleted the feat/accumulator-spec-split branch July 22, 2026 16:51
zzylol added a commit that referenced this pull request Jul 24, 2026
This doc and data_plane/docs/l4node-plan-executor-design.md (Step C,
#409, itself following Step A/#407 and Step B/#408) were written four
days apart and never cross-referenced each other. §6's RoutingIndex
assumed a flat one-query-to-one-materialization model that predates
L4Node's tree shape and doesn't account for SummaryExecutor::find_candidates
(ASAPController#155, the serving-time counterpart to this section).

As originally written, §6 would reintroduce the exact bug AccumulatorSpec
(#401) was built to close: step 5's Capability::is_satisfied_by is
family-level only and can't guarantee two candidates actually share
(SketchKind, SketchParams), which SummaryMerge requires.

Corrects three things:
- Granularity: RoutingIndex's Tier-2 lookup must be invocable per L4Node
  leaf (find_candidates is called once per SummaryAgg, possibly several
  times for one nested query), not only once per whole query.
- Match precision: find_candidates needs exact (SummaryKind,
  SummaryParams) matching via AccumulatorSpec, not family-level
  Capability -- required for anything that can feed a SummaryMerge.
- Selection semantics: find_candidates must return every exact match for
  merge_states to fold, not rank-and-pick-one like the original
  whole-query mode.

Both consumption modes can share the same columnar/interned Tier-2
structure (§6.1) -- only match precision and return shape differ by
caller. No code changes; this is a design-doc correction so
implementation (of either this or #409) doesn't have to be redone once
the two are compared.

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