Skip to content

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

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

zzylol wants to merge 3 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
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
@zzylol
zzylol force-pushed the stack/509-c5-runtime-consistency branch from 8d68e5a to 38e7c90 Compare October 8, 2026 16:47
zzylol added a commit that referenced this pull request Oct 9, 2026
Merge coverage is cached at construction (#646); full per-type overlap
table from #592; k mismatch is a schema rejection; #579 top-k shape;
tighter cost bounds; walk-order caveat for combining conditions; Project
qualifier; absolute time marked later; Example B scope; one-walk wording;
define by[job] and closed schema; link Pass 2; window notation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
@zzylol
zzylol force-pushed the stack/509-c5-runtime-consistency branch from 38e7c90 to 6964909 Compare October 10, 2026 14:32
zzylol and others added 3 commits October 11, 2026 16:22
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/572-b1-types-modules branch from 0babc9d to bd25cf4 Compare October 11, 2026 17:11
@zzylol
zzylol force-pushed the stack/509-c5-runtime-consistency branch from 6964909 to b3f8c6e Compare October 11, 2026 17:11
zzylol added a commit that referenced this pull request Oct 12, 2026
…ion) (#573)

* docs: design for ASAP primitive schema and summary semantics

One design document for the edge schema of ASAP primitives: fields that carry
summary state, the summary semantics a schema must preserve, coverage, and
per-operator examples. Related docs keep a summary and link to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: restore the hand-written sections before the examples

Keep the author's sections 1–4 as written; the operator examples (§5) and
key code interfaces (§6) stay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: a top-k readout returns selected rows

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>

* docs: explain why summary coverage is a node field, not part of the schema

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: summary coverage records rows and columns

SummaryCoverage records which columns a state summarizes (input,
group_by) as well as which rows. Update the SummaryAgg and SummaryMerge
examples to #560: merges compare coverage columns, carry no coverage
until #646, and merged_coverage is gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs: summary coverage as definition + selection, based on Goldstein-Larson view matching

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: the 5.1 example's time index is a Timestamp column

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: absolute time is a timestamp-column interval; match the coverage interface in #646

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: ambiguous column names and literal types in selections

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: fill §1 links and operator list, §2 terminology, §3 summary type terms

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: point links into asap-primitive-schema.md at its current sections

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: list the levels of a summary type as a table

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: list supported query intents per summary type

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: structure §4 with summary table, rule tables and bullet lists

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: rename §4.2 heading

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: explain the residual condition of view matching

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: define the three parts of a view's WHERE with examples

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: define equivalence classes, ranges and residuals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: move the Goldstein & Larson mapping after §4.2 as §4.3

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: say which paper requirement unions of views relax

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: SummaryMerge is implemented on main since #560

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: §4.7 leaves the deployment runtime, such as SDS, out of coverage

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: rewrite §4.1 as a worked G&L example and §4.2–§4.5 in plain terms

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: worked examples for §4.4 and §4.5; drop the §5.8 recap table

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: draw the §5 examples as diagrams and timelines

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: reword how coverage is computed in §4.4

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: explain the selection derivation in §4.4 as goal and steps

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: move the Goldstein & Larson mapping into a footnote of §4.2

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: clearer wording for Rule 1

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: draw §5 examples as bottom-up operator trees

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: draw §4.4 and §5 examples as DAGs with shared nodes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: rename step 3 in §4.3

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: phrase step 3 outcomes as conditions

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: plainer Rule 1 and Rule 2 details with reasons and examples

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: §4.3 derives the definition first, then the selection

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: name the code interface in the §4.5 heading

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: move the coverage code interface to §6.5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: §6 intro points to §6.5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: merge §4.2 and §4.3 (why two parts, then derivation); fold §4.4 into §5.6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: refer to the downstream deployment runtime, not a specific backend

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: same wording in the §2 layout row

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: leave the SDS definition as a TODO for a separate doc

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: make §5 easier to read with one question table per operator

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: explain every struct and variant field in §6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: label §5 operator descriptions as operator definitions

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: explain every function in §6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: describe SketchKind.category as the aggregation intents it answers

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: explain what §6.3 update input and readouts are for, with examples

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: rename §6.2 to summary-family parameters

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: show how the §6 code interfaces connect

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: order §6 subsections from the DAG node inward

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: add the cost of deriving coverage (§4.2.3)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: fix correctness issues found in review

Column identity follows the SummaryAgg's input schema (a Project renames
and drops the table); mark §6.1/§6.6 as #646 and per-family overlap as #592;
panes come from window composition (Pass 2); add IRate; three selection
shapes; current SummaryEstimate top-k output; time row of the definition
table; drop unchecked paper section numbers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: fix second-review findings

Merge coverage is cached at construction (#646); full per-type overlap
table from #592; k mismatch is a schema rejection; #579 top-k shape;
tighter cost bounds; walk-order caveat for combining conditions; Project
qualifier; absolute time marked later; Example B scope; one-walk wording;
define by[job] and closed schema; link Pass 2; window notation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: fix §1 grammar; start every example from its PromQL or SQL query

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: restore the author's wording in §1, §3 and §4 openings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: show ASAPOp generic over its child reference, as on main

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: describe the union and instant-selector rules as #646 implements them

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: touching value ranges join; instant selectors join the later lifting rules

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: PromQL comparisons by lowering; constants from folded casts

A vector/scalar comparison lowers to a Filter above rate and moves to the
selection; a bool comparison is a Project and a vector/vector comparison a
BinaryOp, both kept in the definition. The SQL front end folds numeric casts
of literals as DataFusion does, so `latency < 100` reads as a constant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

* docs: only a Project may rename on the way to the selection

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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