Repository navigation
fix(optimizer): reuse exact results, grouped aggregates and maintained summaries - #943
Conversation
…ning numerical guards
…ricing is unsupported
zzylol
changed the title
zzylol
left a comment
zzylol
left a comment
There was a problem hiding this comment.
Reviewed the implementation and regression coverage for #922, #923, #924, #925, #902 and #886. This is an author self-review, not independent approval.
Checked that filtered grouped quantile reuse preserves SQL empty/NULL behavior and restricts accuracy propagation to a single exact key; count rollups preserve integer states; raw-row heaps retain membership-accuracy rejection where no guarantee exists; restricted/structural search seeds are evaluated at actual cost and do not supply a global lower bound. Numerical safety thresholds remain unchanged.
The first CI run exposed CPU contention between concurrent wall-clock planning regressions. Commit acdccb8 serializes planning within each affected test binary without changing the 30-second budgets or cost assertions. Both binaries now pass with default libtest concurrency (14 tests), and formatting passes. Previous focused validation passed 690 tests and workspace clippy. Awaiting the new full CI run before merging.
|
CI follow-up: all 12 sharing regressions passed on GitHub after isolating their planning budgets. The full run then reached an older filter-subquery composition test that assumed SQL count sketch choices were available. Those unsupported choices are deliberately pruned by this PR. Commit 3d634d3 uses a quantile subquery instead, preserving the multiple-choice substitution and approximate-input guarantee assertions. All 14 tests in stage1_logical_asap_optimization pass locally. Full CI is being rerun; no production behavior or numerical threshold changed in these follow-ups. |
|
Further CI coverage found nine legacy candidate-composition tests that depended on unsupported SQL Count CMS/UnivMon choices. ede27c6 updates SQL count assertions to require the exact accumulator and explicit NoRelationalCountSketchRealization reasons. Approximate composition tests now use executable KLL quantiles, retaining filter/HAVING placement, sizing, substitution, schema and guarantee assertions (including identity-only output alias projections). Validation: all 266 logical-asap-optimization tests pass, all 376 plan-selection tests pass, and the changed package passes clippy with -D warnings. The latest GitHub format/lint check also passed. Full workspace CI remains pending. |
|
Review and validation complete for ede27c6. All checks passed: full Rust workspace tests, format/clippy, DAG viewer Python and Node tests, PromQL-only planner checks/tests, MetricsQL external dependency compatibility, vendored parser baseline, and PR title. CI: https://github.com/ProjectASAP/ASAPPlanner/actions/runs/40449306832 Merged via squash as ebfad9c. Verified #922, #923, #924, #925, #902 and #886 are all closed as completed. |
Same selected plans and costs. Only the search report changed: about twice the joint-price solves, and the direct-cost path now validates plans for q19, q22, q23, q25 and q26. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG
#943 fixed #902's literal, ungrouped case and closed the issue. This test's grouped variant (`... AND city = 'city1' GROUP BY city`) holds only since #952's finer-aggregate reuse, which closes #950: q' now adds 0.000040/s against the Filter witness's 0.000010/s, within the 0.001963/s tolerance. The test still expected the violation, so main's CI has failed on finds_issue_902_filter_on_group_key_not_reading_the_grouped_result since b89a602 (#952). As the file header says, its expectation flips to Outcome::Holds and the test stays as the regression test; on failure it prints the outcome. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Same selected plans and costs. Only the search report changed: about twice the joint-price solves, and the direct-cost path now validates plans for q19, q22, q23, q25 and q26. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG
Same selected plans and costs. Only the search report changed: about twice the joint-price solves, and the direct-cost path now validates plans for q19, q22, q23, q25 and q26. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG
* test(devtools): add planner corpus snapshot harness The planner has no reference answer, so pin its output instead: each corpus query's stage_pipeline document (default search) is compared with a committed snapshot. Timings are stripped and a run stopped by the wall-clock budget fails, so snapshots do not depend on machine speed. UPDATE_SNAPSHOTS=1 regenerates them. Part of #941. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG * test(devtools): snapshot 14 o11y-bench queries The o11y-bench queries that default search plans in under 10 s and 500 MB, each on its own, with declared source facts for the corpus metrics. Queries 05, 06 and 11 select no plan and are recorded as failure snapshots. Checked deterministic across two runs and a single-core run. Part of #941. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG * ci: check planner snapshots in a release-build job Runs the corpus snapshot test against a release build, since the corpus queries take minutes in debug, and uploads the regenerated snapshots when a plan changed. Part of #941. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG * docs: explain planner snapshots and require Snapshot changes in PRs A developer note on what the snapshots pin, how to regenerate them and how to add queries, plus a Snapshot changes section in the PR template and AGENTS.md so every changed plan is explained. Part of #941. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG * test(devtools): regenerate o11y-bench snapshots after #943, #961, #959 Same selected plans and costs. Only the search report changed: about twice the joint-price solves, and the direct-cost path now validates plans for q19, q22, q23, q25 and q26. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKYtMxgJTLtPW9uAqPpiEG * test(devtools): snapshot only the selected plan, not the search The corpus snapshots compared the whole stage_pipeline document, including the search report's counters, status and candidate ids. Those vary from run to run because pane-width rounds have wall-clock budgets, so every snapshot failed on main without any plan change. - Snapshot only the selected Stage 2 candidate (display name, DAG), its cost and accepted accuracy, materialized views, pane retention, warnings and whether the search proved it optimal; for a query that does not plan, the error up to its search report and the rejection causes. Floats are rounded to 12 significant digits. The memo is not built (--no-memo). - Fail on a search report with status time_limit, not only on a run that used the whole budget. - 120 s per query instead of 600 s; the CI job has a 45 min timeout and caches only the cargo registry next to sccache. - Mark q18 (#1023) and q21 (a pane round ends on its wall-clock budget) known_slow and skip them. - Clear the regenerated directory at the start of a run, and write each failing query's full document next to its regenerated snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(devtools): 240 s per query, skip_reason, document the final-round status check Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: zzylol <50204836+zzylol@users.noreply.github.com>
Repeated and related queries could rebuild work that another query already computes. This change adds the missing reuse candidates and a numerically safe search fallback for mixed repeat intervals.
Before this PR: duplicate exact percentiles were separate scalar aggregates; coarser groups missed inner range reductions; one-pane readers could not join longer chains; filtered SQL quantiles rebuilt their input; SQL count-ranked TopK lacked a raw-row heap. Direct pricing could reject mixed schedules on numerical-range guards and leave the search with a much more expensive plan.
After this PR:
Key code interfaces added / modified: CSE eligibility; pane/rollup candidate rules;
add_filtered_group_candidates; guarantee recomposition;PruneReason::NoRelationalCountSketchRealization; internal maintained-state and structural search seeds. No executor API changes.Major algorithm changes: bounded maintained-state proposal search with evaluator-only structural fallback; reuse through identical per-series reductions; integer count-state rollups; filtered single-group quantile reads.
Validation: failing-first reproductions, focused production-planner regressions, executor checks for NULL/empty/grouped-count/raw-heap cases, negative accuracy and reuse-boundary tests, and search tests preserving full-domain bounds. 690 focused tests passed:
asap-types --lib(211),asap-logical-asap-optimization --lib(231),asap-plan-selection --lib search::(225), and the three integration targetsidentical_exact_queries,per_computation_candidates_sql, andsharing_issue_regressions(23).cargo fmt --all -- --checkandcargo clippy --workspace --all-targets --all-features --locked -- -D warningsalso passed. Bounded search results are incumbents, not claims of whole-workload optimality.Fixes #922
Fixes #923
Fixes #924
Fixes #925
Fixes #902
Fixes #886