feat(sketch_algebra): unify SketchKind onto asap_sketch::SummaryKind - #400
Merged
Merged
Conversation
Step 3 of the sketch-identity unification (see
scratchpad/artifacts/enum-unification-plan.md). Replaces
sketch_algebra::{SketchKind, SketchParams} (5 approximate-sketch-only
variants, heap-bearing as a with_heap params flag) with
asap_sketch::{SummaryKind, SummaryParams} (14 variants: exact
accumulators + approximate sketches, heap-bearing promoted to kind
identity) across all 24 real consumers, then deletes the now-empty
sketch_params.rs entirely.
Upstream: bumped ASAPController's pin to 150ef7d (merge of PR #142,
"derive PartialOrd/Ord for SummaryKind") -- needed for
BTreeSet<SummaryKind> in the 5-sketch routing-connector emit path's
deterministic-order contract. No serde needed upstream: confirmed by
audit that every real emit path builds JSON/YAML by hand
(build_backend_aggregation_json etc.), never via serde_json::to_value
on a whole struct -- the existing Serialize/Deserialize derives on
PhysicalExpr/BackendAggregation/EdgeSketchProcessor/GatewayMergeProcessor
only ever backed #[cfg(test)] round-trip tests, which are now deleted
(3) or rewritten to check Debug-string substrings / clone-equality
instead (2), preserving what they actually verified.
Behavior-preservation notes (not a pure mechanical rename):
- Wire text is unchanged. build_backend_aggregation_json's JSON shape
(aggregationType/aggregationSubType/parameters) is byte-identical --
sketch_kind_to_backend_type/sketch_params_to_json were rewritten to
read the new shape but produce the same strings/fields, including
the pre-existing quirk where Cms's JSON never carried a with_heap
key (only CountSketch's did) and heap_size was never emitted on this
wire path at all.
- Heap-bearing dispatch. Every function that used to match a bare
SketchKind (Cms/CountSketch) regardless of with_heap now matches
`Cms | CmsWithHeap` / `CountSketch | CountSketchWithHeap` so
topk-bound aggregations keep routing through the same processor
names, merge processors, and warm-shape classification as before.
- The 5-sketch routing-connector path (FAMILY_ORDER, needed_families,
family_to_proc) is bare-5-family-keyed by design (matching the
retired SketchKind's total lack of heap distinction) -- added a
base_family() normalizer so a committed heap-bearing kind still
matches its bare FAMILY_ORDER entry instead of being silently
dropped.
- bind_cms_topk.rs now threads the real requested top-k `k` into
SummaryParams::{Cms,CountSketch}WithHeap.heap_size (previously
unrepresentable -- with_heap was a bool with no size field). The
backend wire path doesn't read this field either way, so this is
strictly more correct in-memory data with no observable behavior
change.
- SketchType <-> SummaryKind conversions (previously
sketch_params::{impl From<SketchKind> for SketchType, impl
From<SketchType> for SketchKind}) moved to types.rs, next to
SketchType's own definition (orphan rule -- the impl must live in a
crate owning one of the two types).
- Test expectations updated (not regressions): several tests asserted
a bare CountSketch/Cms kind for a topk-bound aggregation; the
underlying behavior (heap-bearing wire params) was already correct
before this change, only the enum value identifying it changes.
Also fixes a real bug in five_sketch routing (`needed_families`,
`family_to_proc`, `family_to_keep_processors`) that would have
silently dropped a committed heap-bearing family before base_family()
normalization was added.
- cargo build --workspace: clean
- cargo test -p control_plane: 822 passed, 1 pre-existing unrelated
failure (invalid_sketch_type_override_falls_back_to_default)
- cargo test -p data_plane --lib: 881 passed, unaffected
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
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
Step 3 of the sketch-identity unification (design doc, Steps 1–2: ASAPController#141/#142, PR #399). Replaces
sketch_algebra::{SketchKind, SketchParams}(5 approximate-sketch-only variants, heap-bearing as awith_heapparams flag) withasap_sketch::{SummaryKind, SummaryParams}(14 variants: exact accumulators + approximate sketches, heap-bearing promoted to kind identity) across all 24 real consumers, then deletes the now-emptysketch_params.rsentirely.Upstream: bumped ASAPController's pin to
150ef7d(merge of PR #142, "derivePartialOrd/OrdforSummaryKind") — needed forBTreeSet<SummaryKind>in the 5-sketch routing-connector emit path's deterministic-order contract. No serde needed upstream — confirmed by audit that every real emit path builds JSON/YAML by hand, never viaserde_json::to_valueon a whole struct. The existingSerialize/Deserializederives onPhysicalExpr/BackendAggregation/EdgeSketchProcessor/GatewayMergeProcessoronly ever backed#[cfg(test)]round-trip tests, now deleted (3) or rewritten to checkDebug-string substrings / clone-equality instead (2), preserving what they actually verified.Behavior-preservation notes (not a pure mechanical rename)
build_backend_aggregation_json's JSON shape is byte-identical —sketch_kind_to_backend_type/sketch_params_to_jsonread the new shape but produce the same strings/fields, including the pre-existing quirk whereCms's JSON never carried awith_heapkey (onlyCountSketch's did) andheap_sizewas never emitted on this wire path at all.SketchKind(Cms/CountSketch) regardless ofwith_heapnow matchesCms | CmsWithHeap/CountSketch | CountSketchWithHeapso topk-bound aggregations keep routing through the same processor names, merge processors, and warm-shape classification as before.FAMILY_ORDER,needed_families,family_to_proc) is bare-5-family-keyed by design — added abase_family()normalizer so a committed heap-bearing kind still matches its bareFAMILY_ORDERentry instead of being silently dropped. This fixes a real latent bug: without normalization, a metric committed to a heap-bearing family would have silently vanished from this emit path.bind_cms_topk.rsnow threads the real requested top-kkintoheap_size(previously unrepresentable). The backend wire path doesn't read this field either way, so this is strictly more correct in-memory data with no observable behavior change.SketchType <-> SummaryKindconversions moved totypes.rs, next toSketchType's own definition (orphan rule).CountSketch/Cmskind for a topk-bound aggregation; the underlying behavior (heap-bearing wire params) was already correct before this change, only the enum value identifying it changes.Test plan
cargo build --workspace— cleancargo test -p control_plane— 822 passed, 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default)cargo test -p data_plane --lib— 881 passed, unaffected🤖 Generated with Claude Code