Skip to content

fix(planner): SQL COUNT(*) candidates count rows, not a sample value - #603

Draft
zzylol wants to merge 2 commits into
stack/509-x5b-example4-acceptancefrom
stack/509-x9-pass1-coverage
Draft

zzylol wants to merge 2 commits into
stack/509-x5b-example4-acceptancefrom
stack/509-x9-pass1-coverage

Conversation

@zzylol

@zzylol zzylol commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Stack: Wave 2 chain: #599 → #601 → #600 → #604 → #606 → #603 → #605

Rebased on main d4869a7 (DF 54).

Why

Part of #509 and #580 (Stage 1 coverage). PR #590's acceptance work found that on SQL flows(ts, src_ip), Stage 1 candidates for COUNT(*) … GROUP BY src_ip fail to compose. Pass 1 builds every count update from ColumnRef::SampleValue, the implicit PromQL value column, and SQL rows don't have one. W7b saw the same failure in Stage 2 for exact Count accumulators.

What

#509 Example 2's design queries (distinct, entropy, and the integer L2 that keeps COUNT(*) GROUP BY src_ip as a target), through plan_stages with every candidate displayed:

Stage 1 build failures selected
Before this PR 240 of 400 (ColumnRef::SampleValue has no value column) P5: UnivMon over a non-existent value item. It passes Stages 2–3 but cannot bind.
After this PR 0 of 400 P2: exact Count accumulator, which executes

How

  • logical_candidates::summary_update: on SQL rows (a closed schema without PromQL series identity), COUNT(*) adds Constant(1.0) per row and carries the UnitCount proof.
    • The exact accumulator reads no column.
    • A sketch hashes one non-null column, a grouping column first. The bare count it answers ignores the item's value.
    • PromQL updates are unchanged.
  • Executor: Operator::unit_count_build is an exact count with no value column, so Constant(1.0) binds. SummaryBuild.value becomes Option<usize>.
  • Legacy origin: the same rule as realize_value_frequency_summary_input in pass1/replacement.rs, which uses a unit weight.

Tests

I wrote tests/pass1_sql_coverage.rs first. Both tests failed on the parent, and both pass now:

  • sql_count_star_group_by_candidates_compose_and_execute: every candidate composes and compiles. Pass-through and the Count accumulator execute to a=3, b=2, c=1.
  • example2_design_candidates_all_build: no Stage 1 or Stage 2 build failure.

Not in this PR

Count-Min, CountSketch and UnivMon count candidates still compile but do not bind. The executor's keyed build needs a weight column, so it rejects Constant(1.0), and PromQL count sketches hit the same limit. Stage 3 never selects them here.

Gates

  • cargo fmt --all --check and cargo clippy --workspace --all-targets --all-features -- -D warnings: pass.
  • cargo test --workspace: 1,638 passed, 23 ignored. Example 4 acceptance with materialization #606 had 1,636 / 23, and this PR adds 2 tests.
  • Viewer tests: 29 ran, 6 skipped (Node).
  • The Example 1, 3a and 3b fixtures are unchanged. They are PromQL.
  • Integration with Stage 2 materialization: ingestion vs query time per summary #604: fix: integrate with #604 adds latency_ms: None to the Example 2 coverage test's RootDemand. The Example 2 design queries still build all 400 candidates and select P2.

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the stack/509-x9-pass1-coverage branch from 31ecc84 to 7c0019e Compare October 4, 2026 20:18
@zzylol
zzylol changed the base branch from stack/509-x8a-hydra-kernel to stack/509-x5b-example4-acceptance October 4, 2026 20:18
zzylol and others added 2 commits October 5, 2026 04:48
…value

Pass 1 built every COUNT(*) update from ColumnRef::SampleValue, which SQL
rows (e.g. flows(ts, src_ip)) do not have. Exact Count accumulators failed
to compose, and sketches hashed a non-existent `value` item.

SQL COUNT(*) now adds a unit weight per row; a sketch hashes a non-null
column, a grouping column first. The executor builds an exact count with a
unit weight (Operator::unit_count_build), so the accumulator executes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RootDemand gained latency_ms in #604; the Example 2 coverage test's
demand sets it to None (no latency bound).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the stack/509-x5b-example4-acceptance branch from 25d2af0 to 215ed9e Compare October 5, 2026 06:21
@zzylol
zzylol force-pushed the stack/509-x9-pass1-coverage branch from 7c0019e to 07adadb Compare October 5, 2026 06:21
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