[iceboundrock]

Research for #386 (planning: #281): is Pivot-style exact slot-bound pruning worth building for group by key, count, sort desc, first K? This PR holds the measurement build, the benchmarks, two spikes and benchmarks/topk_agg/REPORT.md. Nothing here changes a shipped build: the hook and the spike compile only with the topk_research cargo feature.

Why there are two spikes

The first spike (96a3b17, results in results/v1/) pruned well on ClickBench but was 6-13% slower, with up to 3.8x the peak memory, on queries it could not prune, and 38% slower at 32 partitions on 8 cores. It recommended No-Go. The interim assessment on the issue traced those regressions to the operator's mechanics, not to the algorithm, and proposed three changes. The second spike implements them:

• A threshold before any merge. T0 is the K-th largest per-slot maximum partial count. One merge of the K slots with the largest such maximum then gives a tighter T1. The kept rows are gathered once, so at most three merges replace up to fourteen doubling waves.
• Bail-out. A partition merges every buffered row in one aggregate when the rows to merge exceed half its input.
• Slots sized from the data: one slot per 128 buffered partial rows instead of a fixed count.

A first rerun showed that bailing out only after buffering still cost 4-10% on unprunable data, because the final merge can no longer overlap the partial aggregation (results/late_bail/). So the spike also gained a probe: after 131,072 rows, if 90% of them lie in slots that cannot be pruned, the partition streams the rest of its input into one merge. Bailing out is always exact, so the probe only affects speed.

Results (one machine, i7-8700K, 8 CPUs; 44 cells, every result checked against DuckDB)

	Spike vs today	Spike vs its control
ClickBench Q16 UserID, K=10, 8 partitions	-23.7%	-28.8%
ClickBench Q34 URL, K=10, 8 partitions	-18.2%	-20.7%
Same queries at 16 / 32 partitions	-17.8% to -31.6%

• The gain is not the "more final partitions" effect. Today's plan also gets faster with more partitions, but the spike's gain against today's plan at the same partition count holds or grows.
• Where nothing can be pruned (uniform, flat and hot-key data, K=1000, 32 partitions), no cell regressed by more than 3.9%. The exception is the 40 ms cells at 1K keys, which reached +4.6% on a 6.3% spread. Peak memory grows by at most 8%, and falls where the spike prunes.

Decision needed

REPORT.md now recommends Go for an off-by-default implementation of exactly this shape (a single COUNT, descending, LIMIT K, over a hash repartition). The decision is the owner's. Before production, the report says it still needs:

• memory-pool accounting and spilling for the buffer;
• a slot count and probe that do not depend on row arrival order. The probe sometimes gives up in a few UserID partitions, and URL at K=1000 gains 14% only with finer slots;
• plan-shape tests against DataFusion upgrades.

Hashing each key a second time is most of what remains of the final stage's cost on URL, about 3% of CPU. The report advises against replacing the repartition operator to avoid it. No follow-up issue is opened.

Review focus

• The exactness argument for T0/T1 and the keep rule (REPORT.md, Phase 2; src/topk_research/prune.rs).
• Whether the evidence supports the changed recommendation, and the limitations: one machine, and probe and slot constants picked before the run, not tuned.

How it was tested

• cargo clippy --all-targets --features topk_research -- -D warnings and cargo clippy --all-targets -- -D warnings pass.
• py-ltseq/tests/test_topk_research.py on a release measurement build: 206 passed. They hold the spike to the unpruned engine across engine and input layouts, ties, NULL, float and string keys, COUNT(col), and fixed and data-sized slots, and check the bail-out and the probe.
• The 1,017 tests of the aggregation, grouping, sort, slice and top_k test files pass with the spike forced on for every eligible plan, at 1 and at 128 rows per slot.
• benchmarks/topk_agg/selfcheck.py passes: 20,000 random inputs for the wave rule and 20,000 for the new thresholds.
• benchmarks/topk_agg/compare.py: 44 cells, all 138 results correct against DuckDB.
• The full test suite was not run locally; CI runs it.

Closes #386

[iceboundrock]

AI Code Review — Round 1

Reviewed HEAD: 9747193fd5990c558e96522e2e73094c7932908b against base main (merge base c1a236aa9b39296ed69afb6b9bd90bf03b11dacf)

Findings

R1-F1 [blocked] src/topk_research/prune.rs:249-253 — Pruning through computed projections suppresses observable errors

Required resolution:

Check every expression in each intervening ProjectionExec, not just the pass-through count column. Reject computed projections unless their evaluation on discarded groups is proven safe, or preserve their required evaluation. Add an error-preservation regression and correct REPORT.md's eligibility argument. Reproduced with four partitions, batch size 64, keys 0..15 repeated 500 times each plus singleton keys 16..20015, shuffled with seed 3: grouped COUNT followed by derive(x=1 // (k - 20000)), count descending, limit 1 raises Divide by zero with pruning disabled, but returns successfully with either 1024 fixed slots or 128 rows per slot. The failing projection remains above the aggregate and never sees the pruned group, contrary to the issue's observable-error requirement.

R1-F2 [non-blocked] src/topk_research/prune.rs:675-717 — Early-bail compute timing includes asynchronous input waits

Required resolution:

Measure active operator work rather than elapsed time across the asynchronous merge. The current subtraction measures time spent inside input.poll_next_unpin, not the suspended intervals after Poll::Pending; those input waits remain charged to elapsed_compute. Probe slotting is also outside this total. Use the inner AggregateExec's trustworthy compute metrics plus explicitly measured probe/operator overhead, or mark early-bail compute attribution unavailable and omit it from CPU-share calculations. Add a delayed/Pending-input regression and update affected report claims/artifacts or clearly invalidate those stage shares. This already affects committed results: uniform_1000000 integer keys at P=8 reports a 101.1% final-stage share. The report's statement that input waiting was subtracted is incorrect; end-to-end latency measurements are separate and unaffected.

R1-F3 [blocked] Issue #386: deliverable requiring findings to be posted back — The final research findings have not been posted back to the issue

Required resolution:

Have the authorized controller/operator post the final second-spike findings and recommendation on #386, cross-reference the report, EPIC #281 and relevant benchmark work, and provide verifiable read-context evidence. The supplied issue contains only the earlier interim assessment, which explicitly leaves the Go/No-Go unsettled; operator-evidence.md contains no entry proving a final update. The committed report and PR description do not satisfy this separate issue-update acceptance criterion. Keep any implementation follow-up conditional on the owner's actual Go decision.

Spec

The PR supplies substantial research evidence: a 162-cell baseline, a 44-cell second-spike comparison, raw results, physical plans, an ownership/bounds argument, an architecture assessment and an explicit scoped Go recommendation. The feature gate preserves the default path. However, exactness is incomplete because eligible post-aggregate projections can lose required errors, early-bail stage attribution is incorrect, and the required final issue update is absent.

Standards

Read AGENTS.md, CLAUDE.md and docs/non-code-rules.md. The isolated feature, Rust physical-plan integration, detached execution hook and candid research limitations fit repository conventions. Feature-enabled clippy and diff whitespace checks pass. Publication is left to the controller; no repository edits or GitHub writes were made.

Assessment

The slot ownership and nonnegative COUNT threshold reasoning appear sound for the narrowly safe shape, and the recorded wall-clock comparisons support further investigation on this machine. The projection counterexample prevents accepting the current exactness claim. Correcting stage attribution need not invalidate independently measured latency, but the research report must distinguish those measurements accurately.

Observations

The controller's PR snapshot matches the bound HEAD and base branch. Buffer memory accounting/spilling, single-execution threshold state and one-machine generalizability are explicitly documented research limitations, not undisclosed production guarantees. The feature-specific Python tests are not executed by the supplied CI workflow's default-build Python step; they were exercised locally during this review. No follow-up issues are recorded.

Verification

Reviewed the bound diff, related code, issue/PR context, reports, raw-result structures and plan artifacts. Controller snapshot: macOS succeeded; build-and-test and memory-harness remained pending; no controller validation commands were configured. Ran cargo clippy --offline --all-targets --features topk_research -- -D warnings successfully. Research tests: 206 passed, 1 skipped using the available measurement build. selfcheck.py: 20,000 wave cases and 20,000 threshold cases passed. Nine related aggregation/grouping/sort/slice/top_k test files: 652 passed with pruning forced at one row per slot, and 652 passed at 128 rows per slot. The first combined invocation timed out during its second configuration; that configuration was rerun to completion. Independently reproduced the projection error discrepancy. Parsed all ten JSON artifacts, verified current and late-bail comparison summaries/Markdown regenerate in memory, verified Phase 1 Markdown against its summary, and matched 81 plan files to embedded raw plans. Stored results contain five measured repetitions and successful checks for all 162 baseline cells and 138 current comparison cell/mode results; the full performance matrix was not rerun. Final HEAD is unchanged and git status is clean.

Summary

Another fix round is required. Fix the reproduced projection error suppression, correct or withdraw early-bail CPU attribution, and complete the final issue-reporting deliverable. Existing focused tests pass but do not cover the failing error-preservation case.

Needs another fix round: YES

[iceboundrock]

AI Code Review — Round 2

Reviewed HEAD: 15d58f39f2619d13016c04b5a2a3ec8553f8c95b against base main (merge base c1a236aa9b39296ed69afb6b9bd90bf03b11dacf)

Findings

None.

Spec

The research artifacts provide the benchmark matrix, correctness argument, architecture assessment, comparison and scoped recommendation. The final findings were posted back to the issue with the requested cross-references. The report discloses that the related issue threads were unavailable during the research run.

Standards

The feature-gated research implementation and report are consistent with the applicable repository guidance. The review was read-only and publication is left to the controller.

Assessment

The revised projection eligibility avoids the previously reproduced error suppression, and early-bail stage shares from the affected measurements are explicitly withheld. The recommendation is appropriately limited by the reported single-machine evidence and the unmeasured production memory and spill work.

Observations

R1-F1 is resolved by rejecting computed intervening projections and adding the error-preservation regression. R1-F2 is resolved by poll-based timing, a delayed-input regression, and withdrawal of affected historical shares. R1-F3 is resolved by the final findings comment on issue #386. The PR snapshot matches the bound HEAD and base. The feature-specific Python tests are not run by the default-build Python CI step.

Verification

Inspected the bound diff, issue and PR context, prior review, report and verification artifacts. All three head checks were pending in the controller snapshot; no controller validation commands were configured. Feature-enabled offline clippy passed. The delayed-input Rust regression passed. The research Python suite passed with 208 passed and 1 skipped, and both 20,000-case randomized self-checks passed. Diff whitespace check passed; the worktree remains clean. The full performance matrix and full test suite were not rerun.

Summary

No findings require another fix round. The earlier three findings are resolved, and focused verification passed.

Needs another fix round: NO

[iceboundrock]

Explainer: what "Go for exactly the shape the spike covers, off by default" means

The report's recommendation is easy to read as wider than it is. This spells out the eligible query shape as the rule at 15d58f3 enforces it (src/topk_research/prune.rs, rewrite_sort / rewrite_below / Shape::of), and why each limit exists.

The shape

SELECT key, COUNT(*) AS cnt   -- or COUNT(col)
FROM t
[WHERE ...]                   -- filtering before the aggregate is fine
GROUP BY key                  -- one or more grouping columns
ORDER BY cnt DESC
LIMIT K

In LTSeq: t.agg(by=lambda r: r.key, cnt=count).sort("cnt", desc=True).slice(0, K). ClickBench Q16 (UserID) and Q34 (URL) are this shape.

In the physical plan, every one of these must hold:

	eligible	not eligible
aggregate	exactly one non-distinct COUNT(*) or COUNT(col)	SUM/AVG/MIN/MAX, COUNT(DISTINCT), a second aggregate next to the count
grouping	one or more keys, a single grouping set	no GROUP BY, grouping sets/rollup, an aggregate FILTER clause
sort	the count alone, descending	ascending; a second sort key (e.g. cnt DESC, key ASC as a tie-break); a computed sort expression
limit	fetch = K, K > 0	no limit
between aggregate and sort	projections of columns and literals only	any computed expression, even one the sort doesn't use (derive(lambda r: {"x": r.cnt * 2}))
plan	Partial → RepartitionExec: Hash → FinalPartitioned	no hash repartition: at one partition the plan is a single AggregateExec: mode=Single, so the rule never applies there

Key types are unrestricted; the tests cover integer, string, float and NULL keys. Anything else keeps today's plan, and test_planned_only_for_the_eligible_shape asserts the counterexamples.

Why each limit

• COUNT only. The bound is sound because partial counts are nonnegative and sum to the final count, so a slot's sum bounds every group in it. A SUM over negative values breaks that. MIN/MAX with a limit are already handled by DataFusion's own TopKAggregation. The issue also asks that other aggregates not be generalized to without a separate proof.
• The count as the only sort key, descending. The threshold is "the K-th largest count". With a second key or another sort expression, it no longer decides which groups can be dropped.
• No computed projection in between. Round 1 of review found this. A projection evaluates on every group the aggregate emits, so an error it raises on a group that would be pruned is observable today. The first version checked only the column carrying the count; 1 // (k - 20000) then never ran on pruned groups, and the query returned a row where today's plan raises division by zero. Every expression must now be one that cannot fail: a column or a literal.
• A hash repartition below the final aggregate. It puts all partial rows of a key in one partition. That is what makes each partition's own top-K exact, and pruning partition-local.

"Off by default"

Today the spike exists only in a measurement build (--features topk_research) and only after _topk_set_prune turns it on; shipped wheels don't contain it. "Go" means building the same narrow shape to production standard behind a setting that defaults to off: with it off, every plan is unchanged; with it on, only queries matching all of the above get the pruning operator.

What that implementation would still owe is listed in the report and in the findings comment on #386: registering the buffer with the memory pool and spilling; slot sizing and the bail-out probe not depending on row arrival order; and plan-shape tests that pin the rule to DataFusion's aggregate and repartition operators across upgrades. If those cost more than the measured margin, or another machine doesn't reproduce the gains, the recommendation goes back to No-Go.

This PR stays open for now and is not to be merged yet.