Skip to content

test(planning): pin count() to series-count, not distinct cardinality - #724

Merged
zzylol merged 1 commit into
mainfrom
fix/count-is-not-distinct-cardinality
Sep 14, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/count-is-not-distinct-cardinality

Conversation

@zzylol

@zzylol zzylol commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Stacked on #700 — base is feat/shared-series-quantiles, not main. Merge #700 first.

The invariant

PromQL count(v) counts series. It is not a distinct-item count, and it must
not be answered by merging HLL registers: for two series holding {a,b,c} and
{d,e,f} that returns 6 — the union's distinct cardinality — where PromQL's
answer is 2.

Why this needs a test

This is not hypothetical. ASAPPlanner ca7546de, the revision main pins
today
, lowers a bare count by (d) (...) to AggIntent::Cardinality, which
capability_for binds to Capability::CardinalityApprox (HLL):

# on main (ca7546de)
count(unique_users)                        -> [Cardinality]   ← wrong
count_over_time(unique_users[5m])          -> [Count]
distinct_over_time(unique_users[5m])       -> [Cardinality]

# on this branch (b8b5d705)
count(unique_users)                        -> [Count]         ← fixed upstream
count by (svc) (unique_users)              -> [Count]
distinct_over_time(unique_users[5m])       -> [Cardinality]
count(distinct_over_time(unique_users[5m]))-> [Count, Cardinality]

Upstream narrowed the mapping in b8b5d705Cardinality is now reserved for
the distinct-count idiom (distinct_over_time, COUNT(DISTINCT ...), or the
inner node of count(distinct_over_time(...))). #700 already carries that bump
as a side effect of needing typed ExactKind::Min.

The mapping has flip-flopped across Planner revisions — both spellings appear in
the vendored checkout history. Nothing currently asserts which one is correct, so
a future bump can reintroduce the conflation silently.

What this adds

No behavior change. Two regression tests, one at each end of the invariant:

  • query_parser::tests::bare_count_is_series_count_not_distinct_cardinality
    bare count(v) / count by (l) (v) carry a Count intent and no
    Cardinality intent; the distinct-count idiom keeps its Cardinality intent.
  • runtime_capability::tests::count_and_cardinality_bind_different_sketch_capabilities
    approximate Count binds FrequencyEstimate, Cardinality binds
    CardinalityApprox, and never the same capability. This restates as an
    executable assertion what the module's own comment on
    capability_for_count_approximate_returns_frequency_estimate already argues in
    prose: "COUNT(DISTINCT) never lowers to Count."

Verification

Checked against both revisions, so the test is known to discriminate:

  • on ca7546de (main's pin) the parser test fails with its intended
    diagnostic: count(unique_users) must not carry a Cardinality intent …
  • on b8b5d705 (this branch) it passes

cargo fmt --check, clippy --workspace --all-targets -D warnings, and
cargo test -p control_plane --lib (755 passed, 0 failed) all green.

Note on provenance

The original #700 branch changed this behavior and rewrote the surrounding tests
without mentioning it in any commit message, deleting the ASAPController#163/#165
rationale comments in the process. That is what made it look like an arbitrary
reversal during the rebase. It was not — it was adopting an upstream Planner fix.
This PR separates the assertion from the feature work and writes down the argument.

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the feat/shared-series-quantiles branch from 98d19a3 to e10c3fc Compare September 14, 2026 01:58
@zzylol
zzylol force-pushed the fix/count-is-not-distinct-cardinality branch from 33ca73e to 14ef7dd Compare September 14, 2026 01:58
PromQL `count(v)` counts series. It is not a distinct-item count, and it
must not be answered by merging HLL registers: for two series holding
{a,b,c} and {d,e,f} that returns 6 -- the union's distinct cardinality --
where PromQL's answer is 2.

That is not hypothetical. ASAPPlanner `ca7546de`, the revision main pins
today, lowers a bare `count by (d) (...)` to `AggIntent::Cardinality`,
which `capability_for` binds to `Capability::CardinalityApprox` (HLL).
Upstream narrowed the mapping in `b8b5d705` -- bare `count` now lowers to
`AggIntent::Count`, and `Cardinality` is reserved for the distinct-count
idiom (`distinct_over_time`, `COUNT(DISTINCT ...)`, or the inner node of
`count(distinct_over_time(...))`). This branch already carries that bump.

Nothing here changes behavior. These are the regression tests for an
invariant that has flip-flopped across Planner revisions, asserted at both
ends so a future bump cannot quietly reintroduce the conflation:

- `query_parser`: bare `count(v)` / `count by (l) (v)` carry a Count intent
  and no Cardinality intent, while the distinct-count idiom keeps its
  Cardinality intent.
- `runtime_capability`: approximate Count binds `FrequencyEstimate` and
  Cardinality binds `CardinalityApprox` -- never the same capability. This
  restates in an executable assertion what the module's own comment on
  `capability_for_count_approximate_returns_frequency_estimate` already
  argues in prose.

Verified against both revisions: the parser test fails on `ca7546de` with
its intended diagnostic and passes on `b8b5d705`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zzylol
zzylol force-pushed the fix/count-is-not-distinct-cardinality branch from 14ef7dd to 6de42cd Compare September 14, 2026 02:28
@zzylol
zzylol changed the base branch from feat/shared-series-quantiles to main September 14, 2026 02:28
@zzylol
zzylol merged commit 982b00c into main Sep 14, 2026
1 of 2 checks passed
@zzylol
zzylol deleted the fix/count-is-not-distinct-cardinality branch September 14, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant