Skip to content

fix(ir): top-k sketch readouts return selected rows; Example 1 expectations follow planner output - #579

Draft
zzylol wants to merge 2 commits into
stack/572-b1-types-modulesfrom
stack/509-c5-runtime-consistency
Draft

zzylol wants to merge 2 commits into
stack/572-b1-types-modulesfrom
stack/509-c5-runtime-consistency

Conversation

@zzylol

@zzylol zzylol commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Rebased on main d4869a7 (DF 54).

Why

#509 Example 1 runs end to end (1 → 24 → 24 → 1), but only 8 of the 24 Stage 2 candidates compiled in the runtime (physical_planner::compile). The acceptance tests still expected the spec's 6 candidates.

What

  1. CountSketch+heap top-k. ASAPOp::output_schema derived SummaryEstimate{TopK} as partition keys + topk Utf8. No runtime operator produces or reads that encoding. The runtime's keyed evaluation, the exact Sort → Limit path and EvaluatePopulation{TopK} all return the selected rows, so keyed_evaluation rejected the IR's schema ("invalid keyed evaluation shape"). The IR now derives the selected rows: partition keys + item identity columns + Float64 value. With this, all three top-k implementations of Q2 return one row per selected series.
  2. Count-Min+heap stays rejected. Q2 ranks sum_over_time of raw samples. Nothing in the workload or catalog declares http_requests_total non-negative, and neither existing proof (UnitCount, ResetAwareCounterDerivative) applies. The legacy rule also leaves this case UnknownOrSigned. Proving it would need a metric-type input (a counter or non-negative declaration) and a matching proof variant. The runtime and Stage 3 rules are unchanged and agree.
  3. Expectations follow the planner (user decision). The acceptance spec and tests now expect 24 → 24 → 1. The exact-accumulator options account for the difference from 6. The spec lists all 24 candidates for manual review.

How

  • crates/types/src/ir/operator/asap.rs: adds ranked_rows_schema for SketchStatistic::TopK. New unit test: structure_contract::topk_readout_derives_selected_rows.
  • Regression test, which fails before the fix: stage2_count_sketch_heap_topk_compiles_in_the_physical_planner.
  • stage2_runtime_compiles_exactly_the_candidates_stage3_finds_valid (replaces the ignored "every candidate compiles" test) checks that the runtime compiles the 16 valid candidates and rejects the 8 CMS+heap candidates, for the same reason Stage 3 gives.
  • Un-ignored: stage1_has_24_candidates_covering_every_combination (was stage1_has_six…), stage2_keeps_every_logical_candidate, stage2_preserves_logical_choices, stage2_exact_topk_is_sort_then_limit (8 exact), stage2_summary_topk_is_build_then_estimate, and stage3_charges_each_node_once, which now covers the priced (valid) candidates.
  • Still ignored, each naming the missing feature: stage1_q2_summary_families_are_heap_sketches_and_hydra (no Hydra), stage1_keeps_independent_and_shared_variants (Pass 2 sharing, Hydra), stage3_shared_input_is_not_costlier (Pass 2 sharing, Hydra).
  • Fixture regenerated with stage_pipeline. Only the CountSketch/CMS estimate node schemas changed.

Before this PR: 8 of 24 Example 1 candidates compile in the runtime. The 8 CountSketch+heap candidates fail on the readout shape, and the 8 CMS+heap candidates fail on the non-negative weight rule. 10 acceptance tests are ignored.

After this PR: 16 of 24 compile, which is every candidate Stage 3 finds valid. The 8 CMS+heap candidates are rejected by both the runtime and Stage 3 for the non-negative weight reason. Stage 3 still selects P20 (all exact, 79.401 cpu ms), and every rejection reason is unchanged. 3 tests are ignored, each for a missing feature.

Follow-up: §5.2 of the docs/asap-primitive-schema branch still says topk Utf8 and should say "selected rows". This PR does not touch that branch.

🤖 Generated with Claude Code

@zzylol zzylol changed the title fix(runtime): consistent top-k sketch readouts; Example 1 expectations follow planner output fix(ir): top-k sketch readouts return selected rows; Example 1 expectations follow planner output Oct 4, 2026
zzylol added a commit that referenced this pull request Oct 4, 2026
Follows #579: the top-k sketch readout derives one row per selected item
instead of a packed Utf8 column, matching the exact Sort → Limit shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol and others added 2 commits October 5, 2026 04:48
SummaryEstimate{TopK} derived `partition keys + topk Utf8`, an encoding
no runtime operator produces. The runtime's keyed evaluation, the exact
Sort -> Limit path and EvaluatePopulation{TopK} all return the selected
rows, so every CountSketch+heap Example 1 candidate failed to compile.
Derive partition keys + item identity columns + Float64 `value`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 6-candidate spec did not account for Pass 1's exact-accumulator
options (user decision). Un-ignore the count-only tests, check that the
runtime compiles exactly the candidates Stage 3 finds valid and rejects
Count-Min + heap for the same reason, and list the 24 candidates in the
acceptance spec for manual review. Hydra and shared-input tests stay
ignored, naming the missing feature.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the stack/509-c5-runtime-consistency branch from 5796e23 to 8d68e5a Compare October 5, 2026 06:21
@zzylol
zzylol force-pushed the stack/572-b1-types-modules branch from dfaa958 to 7eda06e 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