fix(sketch): SummaryExecutor::execute() gets a grouping model - #161
Merged
Merged
Conversation
Fixes #159. `SummaryAgg`'s `by: Vec<ColumnId>` field was passed to `find_candidates` but never used structurally -- `fold_states` merged *everything* `find_candidates` returned into one `State`, and `execute` read out one `Value` per tree. There was no concept anywhere in the walker of "one output series per distinct group value" -- the existing test suite only exercised ungrouped, single-scalar cases. For a deployment whose real traffic is dominated by grouped queries (`quantile by (zone) (...)`, the normal case, not an edge case -- the motivating example is ASAPQuery-backend's `data_plane`), calling `execute()` as originally specified would silently merge every group's state into one blob instead of erroring, since nothing checked for it. Adds `SummaryExecutor::GroupKey: Clone + Ord + Default` -- an opaque, deployment-chosen per-group identity (e.g. a label-value map). `find_candidates` now tags each handle it returns with the group it belongs to; `execute` groups them itself before folding (`fold_states`/`merge_states` only ever combine same-group states), and every `ExecOutcome` (`State` or `Value`) is now a per-group list rather than a single value. The ungrouped case (`by` empty, or a `Logical` leaf) is simply a list of one entry under `GroupKey::default()` -- same shape, not a special case. `SummaryMerge`'s `(SummaryKind, SummaryParams)` agreement check stays global across every group from every child (a planning-time property of the node, fixed before any group value is known); the actual state folding happens within each group independently, mirroring how `evaluate_exact_agg`'s existing per-(group, window) fold already works downstream. No consumer depends on the pre-#159 shape yet (#155 just merged, nothing in this workspace or ASAPQuery-backend's `control_plane`/`data_plane` implements the trait yet), so this is a clean breaking change rather than a migration. ## Test plan - [x] `cargo test -p asap-sketch` -- 11 tests (9 existing, adapted to the new per-group-list shape via a small `only()` helper for the single-group assertions, + 2 new: grouped SummaryAgg produces one series per group; grouped SummaryMerge folds within a group, not across groups -- proven by asserting the merged values don't equal the cross-group sum). - [x] `cargo build --workspace` / `cargo test --workspace` -- clean, no other crate touches this trait yet. - [x] `cargo clippy --workspace --all-targets --all-features -- -D warnings` -- clean. - [x] `cargo fmt --all --check` -- clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merged
6 tasks
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
Fixes #159.
SummaryAgg'sby: Vec<ColumnId>field was passed tofind_candidatesbut never used structurally —fold_statesmerged everythingfind_candidatesreturned into oneState, andexecuteread out oneValueper tree. There was no concept anywhere in the walker of "one output series per distinct group value" — the existing test suite only exercised ungrouped, single-scalar cases.For a deployment whose real traffic is dominated by grouped queries (
quantile by (zone) (...), the normal case, not an edge case — the motivating example is ASAPQuery-backend'sdata_plane), callingexecute()as originally specified would silently merge every group's state into one blob instead of erroring, since nothing checked for it.SummaryExecutor::GroupKey: Clone + Ord + Default— an opaque, deployment-chosen per-group identity (e.g. a label-value map).find_candidatesnow tags each handle it returns with the group it belongs to;executegroups them itself before folding (fold_states/merge_statesonly ever combine same-group states).ExecOutcome(StateorValue) is now a per-group list rather than a single value. The ungrouped case (byempty, or aLogicalleaf) is simply a list of one entry underGroupKey::default()— same shape, not a special case.SummaryMerge's(SummaryKind, SummaryParams)agreement check stays global across every group from every child (a planning-time property of the node, fixed before any group value is known); the actual state folding happens within each group independently, mirroring howevaluate_exact_agg's existing per-(group, window)fold already works downstream.No consumer depends on the pre-#159 shape yet (#155 just merged, nothing in this workspace or ASAPQuery-backend's
control_plane/data_planeimplements the trait yet), so this is a clean breaking change rather than a migration.Also updates
docs/l4node-execution-model.md's trait signature and adds a "Grouping" subsection explaining the design.Leaving this unmerged for review — flagging since I (Claude, working with @zzylol) have merge access to this repo; wanted a human to look at the grouping/merge semantics (particularly the "global kind/params check, per-group state fold" split and the "fold whatever's present per group, don't require every
SummaryMergechild to cover every group" choice) before this lands, given it changes the trait every future deployment implements against.Test plan
cargo test -p asap-sketch— 11 tests (9 existing, adapted to the new per-group-list shape via a smallonly()helper for the single-group assertions, + 2 new: groupedSummaryAggproduces one series per group; groupedSummaryMergefolds within a group, not across groups — proven by asserting the merged values don't equal the cross-group sum).cargo build --workspace/cargo test --workspace— clean, no other crate touches this trait yet.cargo clippy --workspace --all-targets --all-features -- -D warnings— clean.cargo fmt --all --check— clean.🤖 Generated with Claude Code