feat: split SummaryDataType into a per-family (Kind, Params) vocabulary - #218
Merged
Merged
Conversation
Issue raised during #217's review: `SummaryDataType::Primitive` was a confusing name (both enum arms are equally "primitive" in the trivial sense of being enum variants), and the underlying `SummaryKind`/ `SummaryParams` pair mixed two axes that don't belong in one type — exact accumulators (Sum/Count/MinMax/Rate/Increase) and approximate sketches (Kll/Cms/Hll/...) were both tagged by the same `SummaryKind`, with `SummaryKind::is_exact()` as the only thing telling them apart at runtime. This splits the vocabulary into five independent families, each with its own `(Kind, Params)` pair: - `ExactKind`/`ExactParams` — the exact, mergeable accumulators. - `SketchKind`/`SketchParams` — the approximate sketches (unchanged set: Kll/Cms/Hll/DDSketch/CmsWithHeap/Kmv/Theta/CountSketch/ CountSketchWithHeap). - `SamplingKind`/`SamplingParams` — new: sampling-based summaries (starts with `Reservoir`). - `WaveletKind`/`WaveletParams` — new: wavelet-transform summaries (starts with `Haar`). - `StatModelKind`/`StatModelParams` — new: fitted statistical/ parametric-model summaries (starts with `Parametric`, deliberately open-ended via a `family: String` the deployment interprets — same "core doesn't enumerate every deployment shape" stance `AggIntent::Extension` already takes). `SummaryDataType` is renamed `SummaryFamilyType` and grows one variant per family (`Plain(DataType)` for an ordinary readable value, plus `ExactAggregate`/`Sketch`/`Sample`/`Wavelet`/`StatModel`, each carrying that family's own `(Kind, Params)`) — so a `Sketch(Kll, ...)` can never be confused for a `Sample(...)` at the type level, and `is_exact()` disappears entirely: exactness is now which arm you're in, not a runtime flag. This is a real architecture change (not a rename) with a genuine trade-off, discussed and confirmed before implementing: `CostModel`'s `rank_candidates`/`size_params` were deliberately generic over any `SummaryKind` so a new technique needed zero new trait methods (`cost_model.rs`'s own module doc said as much). Splitting into 5 top-level families breaks that — those two methods stay scoped to `SketchKind`/`SketchParams` specifically (the only family any core `AggIntent` dispatch actually produces today), and a family other than Sketch gets its own trait methods if/when it needs ranking/sizing of its own, rather than overloading `rank_candidates`/`size_params` across incompatible types. `Implementation` (asap-aware-mapping) mirrors `SummaryFamilyType`'s split so `Sample`/`Wavelet`/`StatModel` are real, constructible outcomes — reachable today only via a deployment's own `CostModel::realize_extension` on an `AggIntent::Extension` node, since no core intent maps to them yet, same status as the exact/sketch split before this change. `bind.rs`'s `SummaryAgg`/`SummaryJoin` now carry a single `family: SummaryFamilyType` field (was two: `summary: SummaryKind, params: SummaryParams`) — the natural consolidation once every family already needs the same "is this exact (no estimate) or does it need a SummaryEstimate readout" handling regardless of which family it is. Verified: cargo build/clippy(--all-targets --all-features --locked -- -D warnings)/fmt --all -- --check/test(--locked)/doc --workspace --no-deps all clean. Test count is 60 in asap-types (was 61 — removed the now-meaningless `is_exact_matches_the_enum_declaration_split` test, since `is_exact()` no longer exists as a runtime check); every other crate's count is unchanged and green. `cargo doc` warnings dropped from 23 to 21 (asap-aware-mapping: 17 -> 15) — no new warnings, and all the new cross-links (SamplingKind, WaveletKind, StatModelKind, ...) resolve cleanly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
|
@zzylol Any design choices to discuss here? |
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.
What
Raised during #217's review:
SummaryDataType::Primitivewas aconfusing name (both arms are equally "primitive" in the trivial
enum-variant sense), and the underlying
SummaryKind/SummaryParamspair mixed two axes that don't belong in one type — exact accumulators
(
Sum/Count/MinMax/Rate/Increase) and approximate sketches(
Kll/Cms/Hll/…) were both tagged by the sameSummaryKind, withSummaryKind::is_exact()as the only thing telling them apart atruntime.
This splits the vocabulary into five independent families, each with
its own
(Kind, Params)pair:ExactKind/ExactParams— exact, mergeable accumulators.SketchKind/SketchParams— approximate sketches (unchanged set).SamplingKind/SamplingParams— new: sampling-based summaries(starts with
Reservoir).WaveletKind/WaveletParams— new: wavelet-transform summaries(starts with
Haar).StatModelKind/StatModelParams— new: fitted statistical/parametric-model summaries (starts with
Parametric, deliberatelyopen-ended via a
family: Stringthe deployment interprets — thesame "core doesn't enumerate every deployment shape" stance
AggIntent::Extensionalready takes).SummaryDataTypeis renamedSummaryFamilyTypeand grows one variantper family (
Plain(DataType)for an ordinary readable value, plusExactAggregate/Sketch/Sample/Wavelet/StatModel, each carryingthat family's own
(Kind, Params)) — aSketch(Kll, …)can never beconfused for a
Sample(…)at the type level, andis_exact()disappears entirely: exactness is now which arm you're in, not a
runtime flag.
The trade-off (discussed and confirmed before implementing)
This is a real architecture change, not a rename.
CostModel'srank_candidates/size_paramswere deliberately generic over anySummaryKindso a new technique needed zero new trait methods(
cost_model.rs's own module doc said as much). Splitting into 5top-level families breaks that — those two methods stay scoped to
SketchKind/SketchParamsspecifically (the only family any coreAggIntentdispatch actually produces today), and a family other thanSketchgets its own trait methods if/when it needs ranking/sizing ofits own, rather than overloading
rank_candidates/size_paramsacross incompatible types.
Implementation(asap-aware-mapping) mirrorsSummaryFamilyType'ssplit so
Sample/Wavelet/StatModelare real, constructibleoutcomes — reachable today only via a deployment's own
CostModel::realize_extensionon anAggIntent::Extensionnode,since no core intent maps to them yet (same status the exact/sketch
split already had).
bind.rs'sSummaryAgg/SummaryJoinnow carry a singlefamily: SummaryFamilyTypefield (was two:summary: SummaryKind, params: SummaryParams) — the natural consolidation once every family alreadyneeds the same "exact (no estimate) vs. needs a
SummaryEstimatereadout" handling regardless of which family it is.
Verification
cargo build/clippy --all-targets --all-features --locked -- -D warnings/fmt --all -- --check/test --workspace --locked/doc --workspace --no-depsall clean. Test count is 60 inasap-types(was 61 — removed the now-meaninglessis_exact_matches_the_enum_declaration_splittest, sinceis_exact()no longer exists as a runtime check); every other crate's count is
unchanged and green.
cargo docwarnings dropped from 23 to 21(
asap-aware-mapping: 17 → 15) — no new warnings, and all the newcross-links (
SamplingKind,WaveletKind,StatModelKind, …)resolve cleanly.
🤖 Generated with Claude Code