fix(sid): revert count_over_time → ExactAgg(Sum) mis-routing - #212
Merged
Merged
Conversation
`count_over_time` was routed to the warm-tier ExactAgg path by PRs #200 (capability_for) and #201 (bind_exact_agg) on the theory "count = sum-of-1s". But the data plane has no count accumulator: struct SumAccumulator { sum: f64 } // no count field fn query(&self, statistic, ..) { match statistic { Statistic::Sum | Statistic::Count => Ok(self.sum), // ← both! ... } } `SumAccumulatorUpdater::update_single` does `self.sum += value` — no projection-to-1 anywhere. So a `count_over_time(m[5m])` query matched against an `m` Sum policy returned the **sum of the sample values**, not the count of samples. Silently wrong. This regression was introduced by my PRs #200/#201 — before them `capability_for(Count{Exact})` returned `None` and the query went to the archive engine, which counts correctly. ## Changes - `capability_for(AggIntent::Count { accuracy: Exact })` → `None` (was `Some(ExactAgg(Sum))`). `count_over_time` routes to archive. - `bind_exact_agg` — `Count{Exact}` arm removed; the rule no longer emits `ExactAgg(Sum)` / `ExactAgg(MultipleSum)` for count. - Tests updated: `capability_for_count_exact_routes_to_archive`, `exact_agg_routing_covers_sum_rate_increase_only`, `does_not_bind_count_exact`, `keyed_count_exact_does_not_bind`. `AggIntent::Count { accuracy: Epsilon/EpsilonDelta }` is unchanged — the approximate-count (`count by (...) (count_over_time(...))` distinct-count idiom) still routes to `CardinalityApprox`. ## Follow-up The correct fix — a real `SumCountAccumulator { sum, count }` that answers Sum / Count / Avg — lands with the temporal/spatial-split work. Until then, `count_over_time` and `avg_over_time` are archive-served (correct, just not warm-tier-accelerated). ## Test plan - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` green 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <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
count_over_timewas routed to the warm-tier ExactAgg path by PRs #200/#201 on the theory "count = sum-of-1s". But the data plane has no count accumulator —SumAccumulatoronly trackssum: f64and itsqueryreturns thatsumfor bothStatistic::SumandStatistic::Count. Socount_over_time(m[5m])matched against anmSum policy returned the sum of sample values, not the sample count. Silently wrong.This is a regression my own PRs #200/#201 introduced. Before them,
capability_for(Count{Exact})returnedNone→ archive engine → correct count.Changes
capability_for(Count{Exact})→None(archive routing)bind_exact_agg—Count{Exact}arm removedCount{Epsilon}(thecount by (...)distinct-count idiom →CardinalityApprox) is unchanged.Follow-up
The real fix — a
SumCountAccumulatoranswering Sum/Count/Avg — lands with the temporal/spatial-split work.Test plan
cargo check --workspacecleancargo test --workspace --lib --binsgreen🤖 Generated with Claude Code