feat(asap_types): retire WindowType in favor of asap_ir::WindowKind - #405
Merged
Merged
Conversation
WindowType (Tumbling/Sliding) was a local duplicate of the same concept ASAPController's IR already models as WindowKind (Tumbling/Sliding/Session), just missing the Session variant and the serving-time traits (Copy/Default/Hash/Display/FromStr, snake_case serde) this workspace's real call sites need. ASAPController PR #143 added those upstream; this bumps the pinned asap-ir/asap-sketch/ asap-plan rev to the merge commit and re-exports WindowKind from asap_types::enums in WindowType's place. First-ever asap-ir dependency for asap_types (and transitively data_plane) -- previously only control_plane depended on ASAPController crates. WindowType lived in the shared crate and had ~35 real data_plane call sites, so unifying it for real (not just adding a Session variant locally) meant crossing that boundary. Pure rename at every call site -- same Tumbling default, same lowercase Display/FromStr round-trip, same wire format. No logic changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 21, 2026
…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>
6 tasks
zzylol
added a commit
that referenced
this pull request
Jul 22, 2026
…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
added a commit
that referenced
this pull request
Jul 22, 2026
…ed params + grouping) (#401) * feat(asap_types, data_plane): split AggregationType into AccumulatorSpec 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> * fix(asap_types): repoint accumulator_spec.rs import after promql_utilities 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> * fix(asap_types): rename WindowType to WindowKind in accumulator_spec.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> --------- Co-authored-by: Claude Sonnet 5 <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
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
WindowType(Tumbling/Sliding) was a local duplicate ofasap_ir::intent_algebra::query_expr::WindowKind(Tumbling/Sliding/Session) -- same concept, this workspace's copy just lacked theSessionvariant.Copy/Default/Hash/Display/FromStr,#[serde(rename_all = "snake_case")]) directly toWindowKind.asap-ir/asap-sketch/asap-planrev to that merge commit (7fcaf914d87e71407c3a6d7ccac613b867f9c11b), addsasap-iras a new dependency ofasap_types, and re-exportsWindowKindfromasap_types::enumsinWindowType's place. All ~35 real call sites renamedWindowType->WindowKind.asap_types(and transitivelydata_plane)'s first-ever dependency on an ASAPController crate -- previously onlycontrol_planedepended on them.WindowTypelived in the shared crate with realdata_planecall sites, so a genuine merge (not just addingSessionlocally) meant crossing that boundary; discussed and confirmed with the user before making the ASAPController-side change.Tumblingdefault, same lowercaseDisplay/FromStrround-trip, same wire format (AggregationConfig's direct-derive JSON/YAML and the hand-rolled"windowType"field both unaffected). No logic changed.Test plan
cargo build --workspace-- cleancargo test -p asap_types -p control_plane -p data_plane --no-run-- all lib/bin/test/bench targets compilecargo test -p asap_types --lib-- 36 passed, 0 failedcargo test -p control_plane --lib-- 822 passed, 1 pre-existing unrelated failure (confirmed also failing onmainpre-change)cargo test -p data_plane --lib -- --test-threads=1-- 881 passed, 0 failed, 2 ignored (7 failures under default parallel threading were confirmed to be disk-I/O test-isolation flakiness on this sandbox, not a regression -- each passes individually and the full suite passes single-threaded)rustfmtapplied to touched files only🤖 Generated with Claude Code