Skip to content

test(devtools): #902 is fixed; its planner_fuzz case now holds - #992

Open
GordonYuanyc wants to merge 1 commit into
mainfrom
test/902-holds
Open

GordonYuanyc wants to merge 1 commit into
mainfrom
test/902-holds

Conversation

@GordonYuanyc

@GordonYuanyc GordonYuanyc commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Status: this PR alone does not make main's CI green. It fixes the first failing test only. CI's cargo test --workspace stops at the first failing test binary, and with planner_fuzz fixed, this PR's CI now reaches crates/devtools/tests/stage_pipeline.rs, where two more tests fail on main:

  • scans_projecting_different_labels_are_shared (stage_pipeline.rs:1821)
  • workload_file_with_a_range_evaluation_is_planned (stage_pipeline.rs:1164, "historical retained-pane feasible cost: 719154.73…")

Both come from main, since this PR touches only planner_fuzz.rs. Earlier main runs never reached them.

A full --no-fail-fast run of main 249ebd23 with this PR: 2,306 passed, 2 failed, 31 ignored. The two failures are code regressions, not stale tests:

Main's CI is green only once this PR and fixes for both issues land.

Why

Since b89a602 (#952), main's CI has stopped at finds_issue_902_filter_on_group_key_not_reading_the_grouped_result in crates/devtools/tests/planner_fuzz.rs, with expected #902's violation, got Holds(...). It fails on every main run and every PR, because CI tests the PR merged into main. Later test binaries never ran (see Status above).

The test was an open-bug test that expected #902's violation:

What

Test only. Following the file header's rule for fixed bugs, the test now expects Outcome::Holds and stays as the regression test. On failure it prints the outcome, as the other issue tests do. The doc comment says which PRs fixed what.

How

check(Relation::FilterOnGroupKey, workload, 1) must return Outcome::Holds(_).

Key code interfaces added / modified

None.

Major algorithm changes

None.

Before this PR

The test expected Violated and failed on main since #952. Every PR's test job failed with it.

After this PR

The test expects Holds and guards the grouped variant against a regression.

Evidence

Execution: with this change, the relation holds. q' adds 0.000040/s against the Filter witness's 0.000010/s, within the 0.001963/s tolerance. The same numbers appear in main's failing CI runs (b89a602, f4eac00, f06a555). Before #952, at 26b6a14, the test passed, meaning the violation was still found.

Verification

  • cargo test -p asap-devtools --locked --test planner_fuzz: 16 passed, 0 failed, before the final amend.
  • After the amend, finds_issue_902 was re-run alone and passed. The amend changed only this test's assertion message and doc comment.
  • cargo fmt --all -- --check: clean.

Architectural decisions

  • Kept the test name, as the header says to keep the test.
  • Checked only the Holds variant, which is how the file's other Holds case is written. Holds does not inspect the plan shape: it bounds q''s marginal cost by the witness's derive cost plus 1% of W's cost.

Limitations and follow-up

possibly_known_issue (crates/devtools/src/planner_fuzz/report.rs:360) still tags every SQL b-filter-on-group-key violation as the open issue #902, which is now closed. Not changed here.

Human review — do not complete with an agent

  • The MVP boundary is correct.
  • New conceptual layers or public interfaces are necessary.
  • The before/after description matches the intended product behavior.
  • Human reviewer:
  • Decision and rationale:

🤖 Generated with Claude Code

#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant