diff --git a/_project/DONE/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml b/_project/DONE/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml new file mode 100644 index 0000000000..f932c24fb1 --- /dev/null +++ b/_project/DONE/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml @@ -0,0 +1,391 @@ +id: tpch-throughput-seed-validation-fix +title: "Fix TPC-H Q11/16/18/20 row-count validation under non-reference seeds so throughput runs can be clean" +worktree: uat-hardening +priority: High +status: Completed +completed_date: "2026-07-11" +category: "Correctness / Data Integrity" + +description: | + DECISION (user, 2026-07-11): fix this as part of the UAT hardening effort. + It is the single defect that neutralizes the nightly throughput UAT gate. + + The defect (documented in .github/workflows/nightly.yml:580-595 and + tests/uat/throughput.py:116-129, verified on develop @ fc31f3604): + TPC-H throughput mode derives per-stream/per-position query parameters as + `seed + stream_id * 1000 + position` + (benchbox/core/tpch/throughput_test.py), so queries 11/16/18/20 -- TPC-H's + parameter-sensitive, answer-set-boundary queries -- never land on the + reference-seed exact-match validation path after the first query of + stream 0. They then fail BenchBox's LOOSE (+-50%) row-count check on + every stream. Reproduced upstream with a custom seed, a different custom + seed, and no seed at all; confirmed independent of concurrency (same 4 + queries fail in single-stream POWER mode with a non-reference seed). + + Consequence: every throughput cell is permanently red, which forced + `continue-on-error: true` on the nightly sweep (nightly.yml:596) and + reduced the CI signal to a single stream-count assertion. Restoring the + gate (nightly-throughput-gate-restoration) depends on this fix. + + Prior-art policy to mirror: the bounded correctness gate ALREADY excludes + Q11/Q16/Q18/Q20 "for answer-set boundary sensitivity" + (docs/operations/release-guide.md:33-35); the fix brings the loose + validation path in line with that established policy instead of inventing + a new one. Expected shape: when a query's substitution parameters differ + from the reference seed AND the query is in the documented + parameter-sensitive set, row-count validation records + "not validated (parameter-sensitive, non-reference params)" instead of a + failure; validation status for the run treats these as neither pass nor + fail (mirroring how the correctness gate's exclusion works). Value: a + clean throughput run becomes achievable and Throughput@Size is computed, + un-neutralizing validate_throughput_metric. + +prior_art: + - "docs/operations/release-guide.md:33-35 + make test-correctness-gate -- the existing Q11/16/18/20 exclusion policy and its rationale. Reuse the policy and the query set constant." + - "benchbox/core/tpch/throughput_test.py -- per-stream seed derivation (the mechanism stays; only validation policy changes)." + - "benchbox/core/expected_results/ -- where reference-seed expectations live; check whether a parameter-sensitive query-set constant already exists there before defining one (if it exists, import it; do not duplicate)." + +work: + - id: w0 + summary: "Re-validate the defect at HEAD: non-reference-seed TPC-H power run on DuckDB confirms Q11/16/18/20 fail loose validation; record command, SHA, and failure lines in notes" + status: done + needs: [] + notes: | + PREMISE CORRECTION: SF 0.01 does NOT reproduce -- expected-results + providers only support scale_factor==1.0 + (benchbox/core/expected_results/tpch_results.py get_tpch_expected_results + returns None for SF!=1.0), so at SF 0.01 every query except scale- + independent Q1 gets ValidationMode.SKIP and the run reports + summary.validation="passed" with 66/66 regardless of seed. Re-ran at + SF=1.0 (still cheap, ~4s generate+load) which does carry answer files. + + Command: uv run -- benchbox run --platform duckdb --benchmark tpch + --scale 1 --seed 12345 --phases generate,load,power --output + /tmp/tpch-seed-repro-sf1 --non-interactive + HEAD SHA: 295b645447773a0a8c6ab94a92a91f014be68540 + Result JSON: benchmark_runs/results/tpch_sf1_duckdb_sql_20260711_081042_1a47cb2c.json + + Failure lines (stream=0, run_type=warmup, from the exported queries[] + array -- the CLI's own summary/console output does NOT surface these; + see w1 notes for why): + {"id": "20", "run_type": "warmup", "status": "FAILED", "stream": 0} + {"id": "18", "run_type": "warmup", "status": "FAILED", "stream": 0} + {"id": "16", "run_type": "warmup", "status": "FAILED", "stream": 0} + {"id": "11", "run_type": "warmup", "status": "FAILED", "stream": 0} + + Also ran the THROUGHPUT phase (2 streams, default base_seed=42, SF=1, + reusing the same generated data): 8 FAILED queries = exactly {11,16,18,20} + x 2 streams (both stream 0 and stream 1), confirming the TODO's "fails + on every stream" framing. CLI surfaced this one as "PARTIAL" / + "8 failed queries" (throughput DOES propagate failures to the run + verdict, unlike power's warmup-only failure which is silently + swallowed -- see w1 notes point 4). + + Premise otherwise CONFIRMED: proceeding to w1/w2. + - id: w1 + summary: "Locate the loose validation path and reference-seed exact-match branch; record the pinned file list in work notes -- w2 must stay within that narrowed set, not the full scope_limit" + status: done + needs: [w0] + notes: | + Seam, precisely: + 1. benchbox/core/validation/query_validation.py QueryValidator. + validate_query_result() is the single dispatch point for row-count + validation (EXACT/RANGE/LOOSE/SKIP) -- called from ~20 platform + adapter execute_query() implementations (benchbox/platforms/*, out + of scope_limit) plus benchbox/core/benchmark_mixins.py (also out of + scope_limit, core/ but not core/validation|tpch|expected_results| + results/). None of those call sites are touched by this fix. + 2. benchbox/core/expected_results/registry.py + ExpectedResultsRegistry.get_expected_result() decides EXACT vs SKIP + purely from stream_id (>0 => SKIP, "no answer files for streams>0") + -- it has NO visibility into which seed produced the current query's + parameters. + 3. benchbox/core/expected_results/tpch_results.py + get_tpch_expected_results() registers EVERY TPC-H query with + ValidationMode.EXACT unconditionally (never LOOSE) -- so the LOOSE + mode described in that file's own module docstring ("BenchBox + automatically switches to LOOSE validation" for custom seeds) is + DEAD in practice. + 4. benchbox/core/tpch/power_test.py TPCHPowerTest.__init__ computes an + `actual_validation_mode` string ("exact"/"loose"/"disabled") from + comparing the seed to get_reference_seed(scale_factor) (lines + ~140-195), stores it on TPCHPowerTestConfig.validation_mode -- but + grep confirms `.validation_mode` is read ONLY when building + to_dict() metadata; it is NEVER consulted to change how + validate_query_result() is called. The "auto-switch to loose" + plumbing is entirely vestigial. Root cause is NOT "wrong tolerance + chosen" -- it's "no seed-awareness reaches the validator at all". + 5. Confirms exact match to TODO's "queries 11/16/18/20 ... never land + on the reference-seed exact-match path" framing: registry.py only + ever exact-validates stream_id in (None, 0); power_test.py's own + warmup/measurement iteration renumbering + (benchbox/core/tpch/platform_power.py current_stream_id = i for + warmup, warm_up_iterations+i for measurement) means only the + WARMUP run is ever tagged stream_id=0 and reaches EXACT validation + at all -- and platform_power.py's warmup loop (lines ~104-129) + never checks power_test_result.success, so those failures are + silently absorbed and never surface in the CLI's printed + success/PASSED verdict (only the measurement runs, tagged stream 1-3 + and therefore already SKIPped, are checked). This means today, + POWER mode's Q11/16/18/20 failures are invisible unless you inspect + the exported JSON's queries[] array directly -- worth flagging as a + separate follow-up (spawned as a background task, not fixed here; + out of this TODO's scope which is validation-policy-only). + 6. SEPARATE BUG FOUND (not fixed here, see below): throughput_test.py + _execute_stream()'s `connection.set_query_context(query_id)` call + (line ~475) omits `stream_id=stream_id` -- so EVERY throughput + stream is treated as stream_id=None -> "stream 0" by the registry, + meaning ALL streams (not just stream 0) attempt EXACT validation. + This is WHY w0's throughput repro found stream 1 also failing + Q11/16/18/20, not just stream 0. Chose NOT to fix this omission in + this TODO: the parameter-sensitive exclusion (w2) resolves it + completely regardless, because the exclusion check does not depend + on stream_id or the registry's skip logic -- it is computed + independently from the actual per-query seed + (base_seed + stream_id*1000 + position) vs the reference seed. + Leaving the stream_id omission in place does not reintroduce any + failure for THIS defect (confirmed: the other 18 "answer-stable" + queries already tolerate stream>0's different derived parameters + without EXACT mismatches in the w0 throughput repro -- 8/44 failed, + all and only {11,16,18,20} x 2 streams). Flagged as a spawn_task + follow-up per review-protocol instead of silently leaving it + undocumented. + 7. No existing PARAMETER_SENSITIVE/BOUNDARY_SENSITIVE Python constant + in benchbox/core/expected_results/ (confirmed via grep) -- the only + prior art is Makefile's CORRECTNESS_GATE_QUERY_IDS (the INCLUDED 18, + inverse of the excluded 4), consumed only by + tests/integration/test_local_platform_benchmark_matrix.py via the + BENCHBOX_CORRECTNESS_GATE_QUERY_IDS env var -- test-only plumbing, + not importable production code. Defining a new constant, not + duplicating. + + PINNED FILE LIST for w2 (narrowed from the full scope_limit): + - benchbox/core/expected_results/tpch_results.py (new constant) + - benchbox/core/validation/query_validation.py (context channel + + exclusion check) + - benchbox/core/tpch/power_test.py (compute + set/clear context) + - benchbox/core/tpch/throughput_test.py (compute + set/clear context) + Design constraint driving the context-channel approach (rather than a + new function parameter threaded through validate_query_result's + callers): threading an explicit "is_reference_seed" argument would + require editing every platform adapter's execute_query() signature + (~20 files under benchbox/platforms/, all outside scope_limit) plus + benchbox/core/benchmark_mixins.py (also outside scope_limit). Instead, + benchbox/core/tpch/{power_test,throughput_test}.py set a thread-local + "is this query using the reference seed's parameters" flag immediately + before calling connection.execute() (the same call site that already + calls set_query_context() via hasattr-duck-typing, so no platform code + needs to know this channel exists), and + benchbox/core/validation/query_validation.py reads it. Defaults to + None (unset) for every caller that never sets it (TPC-DS, DataFrame + validation, any other QueryValidator user) -- preserves their exact + pre-existing behavior untouched, satisfying must_preserve. + - id: w2 + summary: "Implement the parameter-sensitive exclusion: one shared query-set constant, applied only when params differ from the reference seed; per-query detail records the exclusion reason" + status: done + needs: [w1] + notes: | + Implemented exactly the four pinned files from w1, no others: + - benchbox/core/expected_results/tpch_results.py: added + PARAMETER_SENSITIVE_QUERY_IDS = frozenset({"11","16","18","20"}) + (no prior Python constant existed -- confirmed by w1 grep -- so + this is new, not a duplicate of the Makefile's env-var list). + - benchbox/core/validation/query_validation.py: added a + threading.local()-backed context + (set_reference_seed_context/get_reference_seed_context/ + clear_reference_seed_context) plus + get_parameter_sensitive_query_ids(benchmark_type) (dict keyed by + benchmark, tpch -> the new constant, everything else -> empty + frozenset). validate_query_result() checks, BEFORE the registry + lookup: if get_reference_seed_context() is False and the + normalized query_id is parameter-sensitive for this benchmark, + return ValidationResult(is_valid=True, + validation_mode=ValidationMode.SKIP, warning_message="Query 'N' + not validated (parameter-sensitive, non-reference params).") -- + the exact phrase from the TODO description, so it is greppable in + the per-query row_count_validation.warning field + (_build_query_result_with_validation in + benchbox/platforms/base/result_capture.py already maps SKIP -> + status SKIPPED without touching overall query status; unchanged). + - benchbox/core/tpch/power_test.py: TPCHPowerTest.__init__ stashes + self.reference_seed; run()'s per-query loop calls + set_reference_seed_context(stream_seed is None or stream_seed == + self.reference_seed) immediately before connection.execute(), and + clear_reference_seed_context() in the existing finally block + (fires on both success and exception paths). + - benchbox/core/tpch/throughput_test.py: _execute_stream() resolves + reference_seed = get_reference_seed(config.scale_factor) once, + then per query computes stream_seed = seed + stream_id*1000 + + position (same formula _resolve_query_text() already uses to + generate the SQL -- re-read, not re-derived, so + must_preserve's "seed derivation formula unchanged" holds) and + calls set_reference_seed_context(stream_seed == reference_seed) / + clear_reference_seed_context() around connection.execute(). + Deliberately did NOT touch throughput_test.py's + connection.set_query_context(query_id) call (missing stream_id= -- + see w1 note 6): the exclusion above resolves the reproduced defect on + every stream regardless of that separate, pre-existing omission. + Flagged as a spawn_task follow-up, not folded into this change. + ruff, ty, and lint-imports (utils < core < platforms < cli) all clean + on the four touched files; no platforms/*.py file was modified. + - id: w3 + summary: "Verify end-to-end: non-reference-seed run reports clean-with-exclusions; wrong row count on a non-excluded query still fails; summary.validation keeps submit classification honest" + status: done + needs: [w2] + notes: | + Added regression coverage at two levels (no new test files -- all + additions to existing files that already carry pytestmark, so + lint-markers stays clean): + - tests/unit/core/expected_results/test_validation_engine.py + (new TestParameterSensitiveExclusion class, 12 cases): constant + value, get_parameter_sensitive_query_ids() per-benchmark scoping, + context set/get/clear round-trip, each of Q11/16/18/20 excluded + when context=False (asserts the exact warning-message phrase), + each still EXACT-fails when context=True (must_preserve) and when + context is left unset/None (default -- proves callers that never + touch this context, e.g. TPC-DS, are unaffected), a non-boundary + query (Q1) still fails on a wrong count even with context=False + (anti_pattern guard against unconditional exclusion), and TPC-DS + explicitly unaffected by a non-reference context. + - tests/integration/test_tpch_power_test.py (new + TestPowerTestReferenceSeedContext class, 6 cases): verifies + TPCHPowerTest.run() calls set_reference_seed_context with the + correct bool for qgen-defaults / reference-seed / custom-seed + runs, verifies clear_reference_seed_context fires every query, and + two REAL-stack end-to-end cases routed through the actual + PlatformAdapterConnection + DuckDBAdapter + QueryValidator (only + the innermost raw DB cursor mocked, forced to return a row count + that would fail any real SF=1 EXACT comparison): a custom-seed run + reports Q11/16/18/20 as SUCCESS (not FAILED) -- the literal w0 + repro, now fixed; a reference-seed run still reports Q11 as FAILED + (must_preserve). + - tests/integration/test_tpch_throughput_test.py (new + TestThroughputReferenceSeedContext class, 5 cases): verifies + per-position context is True ONLY for stream 0 position 0 with the + reference seed (every other position necessarily diverges, given + the seed+stream_id*1000+position formula), False for the default + base_seed=42 and for any seed at a non-SF=1.0 scale factor (no + pinned reference exists below SF=1.0), context cleared every + query, and a REAL-stack end-to-end case across TWO streams (0 and + 1) confirming Q11/16/18/20 are absent from BOTH streams' failed- + query sets with the default base_seed -- this is the literal w0 + throughput repro (8 failures = {11,16,18,20} x 2 streams), now 0 + boundary-query failures on either stream. (The other 18 + non-boundary queries are expected to fail in this specific test + because the mock forces every query's row count to 999 regardless + of query id -- that is correct EXACT-mode behavior and is not + this test's concern; it only asserts on the boundary set.) + All new + pre-existing tests pass: + - tests/unit/core/ (9993 passed, 12 skipped, 0 failed) + - tests/integration/test_tpch_power_test.py -m "integration and slow" (12 passed) + - tests/integration/test_tpch_throughput_test.py -m "integration and slow" (16 passed) + - tests/integration/test_throughput_corrections.py (included in the + tests/unit/core/ run above via the combined verification command; + separately reconfirmed, all passing) + One incidental finding during verification: a pre-existing local-only + test, tests/unit/core/results/test_integrity_validator.py:: + TestIntegrityValidatorReferenceFiles, opportunistically scans + benchmark_runs/results/*_sf1_duckdb_*_2026*.json (gitignored, dev- + machine-local, no CI equivalent) for the LATEST matching file by + mtime and asserts it doesn't fail integrity checks. My own w0 repro + runs left deliberately-broken result files there, which the test + picked up and failed on (not a regression -- cleaned up the repro + artifacts; benchmark_runs/ is gitignored so nothing to commit). + ruff clean on all three touched test files; lint-imports clean; + lint-markers clean (no new test files, no new markers needed). + - id: w4 + summary: "Run the nightly throughput config locally via make uat-sweep and confirm the cell passes clean with Throughput@Size > 0" + status: done + needs: [w3] + notes: | + Command: make uat-sweep CONFIG=tests/uat/configs/uat-throughput-duckdb-nightly.yaml + (SF=1, streams=3, seed=20260709 -- a non-reference seed -- via + run-official --streams 3, phases_arg="load,throughput", + validator_clean_rate_floor=1.0). Log: + /tmp/tpch-seed-uat-sweep.log. Ran at commit b833c878a (this TODO's + implementation commit); source_dirty=false. + + Result: phase_exit_codes preflight=0 execute=0 validate=0 report=0. + matrix_summary.tsv: status=passed terminal_state=passed + submit_terminal_state=submittable validator_status=clean. + validator_rollup.tsv: error_count=0 warning_count=0 + validator_clean_rate=100.0%. + Result JSON (benchmark_runs/results/tpch_sf1_duckdb_sql_20260711_083610_03c8ab80.json) + summary: queries.failed=0, queries.passed=66 (22 queries x 3 streams), + validation="passed", tpc_metrics.throughput_at_size=265178.57 (>0). + Q11/16/18/20 status=SUCCESS on all 3 streams (0,1,2) -- the literal w0 + defect, now clean end-to-end through the real UAT harness at the + config that previously required continue-on-error in nightly.yml. + + Process note (unrelated to this fix, flagging for the orchestrator): + this worktree's local `develop` ref was stale (merge-base fc31f3604, + well behind the actual develop tip at session start), so `git diff + --name-only develop...HEAD` returned many files from already-merged + upstream PRs. Used `git diff --name-only 295b64544..HEAD` (295b64544 + = this session's recorded starting HEAD SHA, see w0) instead, which + correctly isolates this TODO's own 8 touched files. + +deferred: + - summary: "Parameter-aware expected row counts for Q11/16/18/20 (deriving expectations per seed instead of excluding)" + reason: "Requires per-parameter answer-set computation; the exclusion mirrors the established correctness-gate policy and unblocks the gate now. Track as a future strengthening." + +deps: + needs: [] + +must_preserve: + - "Reference-seed runs keep EXACT-match validation for all queries including Q11/16/18/20 -- the exclusion applies only to non-reference parameters." + - "The correctness gate (make test-correctness-gate) behavior and its exclusion list are untouched." + - "TPC-H/TPC-DS throughput success-gate defaults (0.99/0.70) from PR #1106 unchanged." + - "Per-stream seed derivation formula unchanged (it is TPC-compliant stream permutation behavior)." + +approach: | + Investigation-first TODO: w0/w1 are mandatory before any code. The + scope_limit below is deliberately broad for the INVESTIGATION only; w1 + must pin the actual seam (expected: the validation module producing + summary.validation, possibly tpch/throughput_test.py where seed + provenance is known) and w2+ must stay within the pinned files. Keep the + change scoped to validation policy; do not touch execution. This is + comparator/validation-adjacent code -- check .github/CODEOWNERS: if the + touched paths are soundness-critical, the PR waits for Code Owner review + instead of auto-merge (call it out in the PR body). + +anti_patterns: + - "DO NOT widen the loose tolerance band to make the queries pass -- that weakens validation for every query." + - "DO NOT exclude the 4 queries unconditionally -- reference-seed runs must keep validating them." + - "DO NOT special-case inside tests/uat -- the fix belongs in benchbox core so every consumer (CLI runs, UAT, nightly) benefits." + +verification: + - description: "Non-reference-seed run validates clean with recorded exclusions; wrong-count regression still fails" + command: "uv run -- python -m pytest tests/unit/core/ tests/integration/test_tpch_throughput_test.py tests/integration/test_throughput_corrections.py -q" + expected_output: "pass including new exclusion tests" + - description: "Throughput UAT cell goes green locally" + command: "make uat-sweep CONFIG=tests/uat/configs/uat-throughput-duckdb-nightly.yaml" + expected_output: "exit 0, matrix summary shows the cell passed" + +scope_limit: + only_modify: + - "benchbox/core/tpch/" + - "benchbox/core/expected_results/" + - "benchbox/core/results/" + - "benchbox/core/validation/" + - "benchbox/validation/" + - "tests/unit/core/" + - "tests/integration/" + - "tests/uat/throughput.py" + - "docs/development/throughput-result-alignment.md" + +files_affected: + modified: + - benchbox/core/expected_results/tpch_results.py + - benchbox/core/tpch/power_test.py + - benchbox/core/tpch/throughput_test.py + - benchbox/core/validation/query_validation.py + - tests/integration/test_tpch_power_test.py + - tests/integration/test_tpch_throughput_test.py + - tests/unit/core/expected_results/test_validation_engine.py + +metadata: + tags: [tpch, throughput, validation, seed-derivation, correctness] + estimated_effort: Large + created_date: "2026-07-11" + source: "UAT infrastructure review 2026-07-10 (develop @ fc31f3604); nightly.yml:580-595 known-defect block; user decision Q3" diff --git a/_project/TODO/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml b/_project/TODO/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml deleted file mode 100644 index 3a8e87d6c0..0000000000 --- a/_project/TODO/uat-hardening/planning/tpch-throughput-seed-validation-fix.yaml +++ /dev/null @@ -1,122 +0,0 @@ -id: tpch-throughput-seed-validation-fix -title: "Fix TPC-H Q11/16/18/20 row-count validation under non-reference seeds so throughput runs can be clean" -worktree: uat-hardening -priority: High -status: Not Started -category: "Correctness / Data Integrity" - -description: | - DECISION (user, 2026-07-11): fix this as part of the UAT hardening effort. - It is the single defect that neutralizes the nightly throughput UAT gate. - - The defect (documented in .github/workflows/nightly.yml:580-595 and - tests/uat/throughput.py:116-129, verified on develop @ fc31f3604): - TPC-H throughput mode derives per-stream/per-position query parameters as - `seed + stream_id * 1000 + position` - (benchbox/core/tpch/throughput_test.py), so queries 11/16/18/20 -- TPC-H's - parameter-sensitive, answer-set-boundary queries -- never land on the - reference-seed exact-match validation path after the first query of - stream 0. They then fail BenchBox's LOOSE (+-50%) row-count check on - every stream. Reproduced upstream with a custom seed, a different custom - seed, and no seed at all; confirmed independent of concurrency (same 4 - queries fail in single-stream POWER mode with a non-reference seed). - - Consequence: every throughput cell is permanently red, which forced - `continue-on-error: true` on the nightly sweep (nightly.yml:596) and - reduced the CI signal to a single stream-count assertion. Restoring the - gate (nightly-throughput-gate-restoration) depends on this fix. - - Prior-art policy to mirror: the bounded correctness gate ALREADY excludes - Q11/Q16/Q18/Q20 "for answer-set boundary sensitivity" - (docs/operations/release-guide.md:33-35); the fix brings the loose - validation path in line with that established policy instead of inventing - a new one. Expected shape: when a query's substitution parameters differ - from the reference seed AND the query is in the documented - parameter-sensitive set, row-count validation records - "not validated (parameter-sensitive, non-reference params)" instead of a - failure; validation status for the run treats these as neither pass nor - fail (mirroring how the correctness gate's exclusion works). Value: a - clean throughput run becomes achievable and Throughput@Size is computed, - un-neutralizing validate_throughput_metric. - -prior_art: - - "docs/operations/release-guide.md:33-35 + make test-correctness-gate -- the existing Q11/16/18/20 exclusion policy and its rationale. Reuse the policy and the query set constant." - - "benchbox/core/tpch/throughput_test.py -- per-stream seed derivation (the mechanism stays; only validation policy changes)." - - "benchbox/core/expected_results/ -- where reference-seed expectations live; check whether a parameter-sensitive query-set constant already exists there before defining one (if it exists, import it; do not duplicate)." - -work: - - id: w0 - summary: "Re-validate the defect at HEAD: non-reference-seed TPC-H power run on DuckDB confirms Q11/16/18/20 fail loose validation; record command, SHA, and failure lines in notes" - status: pending - needs: [] - - id: w1 - summary: "Locate the loose validation path and reference-seed exact-match branch; record the pinned file list in work notes -- w2 must stay within that narrowed set, not the full scope_limit" - status: pending - needs: [w0] - - id: w2 - summary: "Implement the parameter-sensitive exclusion: one shared query-set constant, applied only when params differ from the reference seed; per-query detail records the exclusion reason" - status: pending - needs: [w1] - - id: w3 - summary: "Verify end-to-end: non-reference-seed run reports clean-with-exclusions; wrong row count on a non-excluded query still fails; summary.validation keeps submit classification honest" - status: pending - needs: [w2] - - id: w4 - summary: "Run the nightly throughput config locally via make uat-sweep and confirm the cell passes clean with Throughput@Size > 0" - status: pending - needs: [w3] - -deferred: - - summary: "Parameter-aware expected row counts for Q11/16/18/20 (deriving expectations per seed instead of excluding)" - reason: "Requires per-parameter answer-set computation; the exclusion mirrors the established correctness-gate policy and unblocks the gate now. Track as a future strengthening. Promoted to [[tpch-throughput-parameter-aware-rowcounts]] (2026-07-16), which needs this item." - -deps: - needs: [] - -must_preserve: - - "Reference-seed runs keep EXACT-match validation for all queries including Q11/16/18/20 -- the exclusion applies only to non-reference parameters." - - "The correctness gate (make test-correctness-gate) behavior and its exclusion list are untouched." - - "TPC-H/TPC-DS throughput success-gate defaults (0.99/0.70) from PR #1106 unchanged." - - "Per-stream seed derivation formula unchanged (it is TPC-compliant stream permutation behavior)." - -approach: | - Investigation-first TODO: w0/w1 are mandatory before any code. The - scope_limit below is deliberately broad for the INVESTIGATION only; w1 - must pin the actual seam (expected: the validation module producing - summary.validation, possibly tpch/throughput_test.py where seed - provenance is known) and w2+ must stay within the pinned files. Keep the - change scoped to validation policy; do not touch execution. This is - comparator/validation-adjacent code -- check .github/CODEOWNERS: if the - touched paths are soundness-critical, the PR waits for Code Owner review - instead of auto-merge (call it out in the PR body). - -anti_patterns: - - "DO NOT widen the loose tolerance band to make the queries pass -- that weakens validation for every query." - - "DO NOT exclude the 4 queries unconditionally -- reference-seed runs must keep validating them." - - "DO NOT special-case inside tests/uat -- the fix belongs in benchbox core so every consumer (CLI runs, UAT, nightly) benefits." - -verification: - - description: "Non-reference-seed run validates clean with recorded exclusions; wrong-count regression still fails" - command: "uv run -- python -m pytest tests/unit/core/ tests/integration/test_tpch_throughput_test.py tests/integration/test_throughput_corrections.py -q" - expected_output: "pass including new exclusion tests" - - description: "Throughput UAT cell goes green locally" - command: "make uat-sweep CONFIG=tests/uat/configs/uat-throughput-duckdb-nightly.yaml" - expected_output: "exit 0, matrix summary shows the cell passed" - -scope_limit: - only_modify: - - "benchbox/core/tpch/" - - "benchbox/core/expected_results/" - - "benchbox/core/results/" - - "benchbox/core/validation/" - - "benchbox/validation/" - - "tests/unit/core/" - - "tests/integration/" - - "tests/uat/throughput.py" - - "docs/development/throughput-result-alignment.md" - -metadata: - tags: [tpch, throughput, validation, seed-derivation, correctness] - estimated_effort: Large - created_date: "2026-07-11" - source: "UAT infrastructure review 2026-07-10 (develop @ fc31f3604); nightly.yml:580-595 known-defect block; user decision Q3" diff --git a/_project/config/fast_test_lane_policy.json b/_project/config/fast_test_lane_policy.json index e9d4c7ab71..de4d50f626 100644 --- a/_project/config/fast_test_lane_policy.json +++ b/_project/config/fast_test_lane_policy.json @@ -1,7 +1,7 @@ { "enabled": true, - "max_fast_tests": 25190, - "_ceiling_note": "Bumped 13200 -> 22000 on 2026-04-13 to reflect grown fast-lane (20,184 collected). Bumped 22000 -> 22100 on 2026-05-10 for joinorder-canonical-foundation Phase-1 (data_fetch + scale-factor + surface-field tests; 22038 collected). Bumped 22100 -> 22400 on 2026-05-11 for joinorder-canonical-cutover canonical SQL/DataFrame, MCP, publishing, and corpus-inventory guards; 22317 collected. Bumped 22400 -> 22500 on 2026-05-13 for UAT enabled-platform remediation coverage; pr-preflight collected 22419. Bumped 22500 -> 22510 on 2026-05-15 after rebasing onto develop at 36ab01058; pr-preflight collected 22501 before this branch's new public theme contract tests, which are medium-marked to avoid fast-lane growth. Bumped 22510 -> 22530 on 2026-05-14 for pr-review-followup batch coverage (joinorder dataframe family, pg_mooncake adapter, landing quickstart validation, explorer read-model retry); pr-preflight collected 22520. Bumped 22530 -> 22540 on 2026-05-15 for results-explorer-post-theme-reconcile count-aware Home/script coverage; CI collected 22537. Bumped 22540 -> 22543 on 2026-05-16 for pr-review-followups coverage on UAT CLI dispatch, UAT release-gate envelope scoping, and develop-post-merge orphan ordering; ci-lint collected 22543. Bumped 22543 -> 22550 on 2026-05-16 for prompts landing Phase 1 CLI hygiene validator/prompt-shape coverage; pr-preflight collected 22550. Bumped 22550 -> 22553 on 2026-05-16 for prompts landing Phase 2 cost-class and registry prompt-safety coverage; timing policy collected 22553. Bumped 22553 -> 22558 on 2026-05-16 for prompts landing Phase 3 platform-option and TPC-DS dsdgen validator coverage; timing_policy_check collected 22558. Bumped 22558 -> 22561 on 2026-05-16 for prompts landing Phase 4 provenance template and capture-plan footer coverage; timing_policy_check collected 22561. Bumped 22561 -> 22563 on 2026-05-16 for prompts landing review fixes covering MCP cost-gating parity and compare summary discipline; targeted lane count increased by two fast tests. Bumped 22563 -> 22568 on 2026-05-16 for prompts landing Phase 5 MCP parity prompt-surface coverage; timing_policy_check collected 22568. Bumped 22568 -> 22570 on 2026-05-16 for prompts landing MCP real-tool parity and platform-option gap follow-up coverage; timing_policy_check collected 22570. Bumped 22570 -> 22572 on 2026-05-19 for pr-review-followups prompt regressions covering credential-before-smoke ordering and smoke-scale dsdgen warning behavior; timing_policy_check collected 22572. Set 22572 -> 25000 on 2026-05-19 by maintainer direction during pr-review-followups cleanup. Ratchet down if the lane contracts; never raise without justification. Bumped 25000 -> 25050 on 2026-07-16 for tuning-mode-vocabulary-and-facet-implementation-20260712 (ADR-2 canonical_mode/tuned-fallback/official-refusal coverage, the PACKAGED_RESOURCE composition pin added when merging develop's #1188 packaged-template tier, and the physical_mechanisms unknown-vs-empty ingest-pipeline regression tests); develop alone collected 24995, this branch's merge collected 25022. Bumped 25050 -> 25060 on 2026-07-17 for tuning-capability-registry-coverage-20260716 (alias-resolution invariant tests and derived generator-coverage drift guards for the 9 newly-registered platforms); CI collected 25052 on the merge ref. Bumped 25060 -> 25080 on 2026-07-17 composing #1198's registry-coverage guards with #1176's provenance/hash export coverage (test_tuning_provenance_export.py + test_requested_config_hash.py); both branches independently bumped from 25050 (#1198: 25052 collected; #1176 pre-compose: 25062 collected); composed merge tree collects 25077. Bumped 25080 -> 25140 on 2026-07-17 for tuning-from-config-forwarding-sweep-20260716 (test_tuning_config_forwarding.py's registry-driven parametrized test, one case per registered platform adapter, verifying from_config forwards tuning_enabled/tuning_config/unified_tuning_configuration/tuning_source/tuning_source_file); timing_policy_check collected 25128. Bumped 25140 -> 25180 on 2026-07-18 for the todo-db-tracker local-SQLite spike (tests/unit/scripts/test_todo_db.py: 49 fast unit tests covering the enforced lifecycle invariants, archive import, and concurrency guards); develop collected 25128, this branch collects 25177. Bumped 25180 -> 25190 on 2026-07-18 composing the todo-db spike branch with develop's post-25128 growth: the branch alone collects 25177 (49 fast tracker tests; the 11 wrapper tests are medium-marked and collect 0 under -m fast), but the PR merge ref collects 25187.", + "max_fast_tests": 25220, + "_ceiling_note": "Bumped 13200 -> 22000 on 2026-04-13 to reflect grown fast-lane (20,184 collected). Bumped 22000 -> 22100 on 2026-05-10 for joinorder-canonical-foundation Phase-1 (data_fetch + scale-factor + surface-field tests; 22038 collected). Bumped 22100 -> 22400 on 2026-05-11 for joinorder-canonical-cutover canonical SQL/DataFrame, MCP, publishing, and corpus-inventory guards; 22317 collected. Bumped 22400 -> 22500 on 2026-05-13 for UAT enabled-platform remediation coverage; pr-preflight collected 22419. Bumped 22500 -> 22510 on 2026-05-15 after rebasing onto develop at 36ab01058; pr-preflight collected 22501 before this branch's new public theme contract tests, which are medium-marked to avoid fast-lane growth. Bumped 22510 -> 22530 on 2026-05-14 for pr-review-followup batch coverage (joinorder dataframe family, pg_mooncake adapter, landing quickstart validation, explorer read-model retry); pr-preflight collected 22520. Bumped 22530 -> 22540 on 2026-05-15 for results-explorer-post-theme-reconcile count-aware Home/script coverage; CI collected 22537. Bumped 22540 -> 22543 on 2026-05-16 for pr-review-followups coverage on UAT CLI dispatch, UAT release-gate envelope scoping, and develop-post-merge orphan ordering; ci-lint collected 22543. Bumped 22543 -> 22550 on 2026-05-16 for prompts landing Phase 1 CLI hygiene validator/prompt-shape coverage; pr-preflight collected 22550. Bumped 22550 -> 22553 on 2026-05-16 for prompts landing Phase 2 cost-class and registry prompt-safety coverage; timing policy collected 22553. Bumped 22553 -> 22558 on 2026-05-16 for prompts landing Phase 3 platform-option and TPC-DS dsdgen validator coverage; timing_policy_check collected 22558. Bumped 22558 -> 22561 on 2026-05-16 for prompts landing Phase 4 provenance template and capture-plan footer coverage; timing_policy_check collected 22561. Bumped 22561 -> 22563 on 2026-05-16 for prompts landing review fixes covering MCP cost-gating parity and compare summary discipline; targeted lane count increased by two fast tests. Bumped 22563 -> 22568 on 2026-05-16 for prompts landing Phase 5 MCP parity prompt-surface coverage; timing_policy_check collected 22568. Bumped 22568 -> 22570 on 2026-05-16 for prompts landing MCP real-tool parity and platform-option gap follow-up coverage; timing_policy_check collected 22570. Bumped 22570 -> 22572 on 2026-05-19 for pr-review-followups prompt regressions covering credential-before-smoke ordering and smoke-scale dsdgen warning behavior; timing_policy_check collected 22572. Set 22572 -> 25000 on 2026-05-19 by maintainer direction during pr-review-followups cleanup. Ratchet down if the lane contracts; never raise without justification. Bumped 25000 -> 25050 on 2026-07-16 for tuning-mode-vocabulary-and-facet-implementation-20260712 (ADR-2 canonical_mode/tuned-fallback/official-refusal coverage, the PACKAGED_RESOURCE composition pin added when merging develop's #1188 packaged-template tier, and the physical_mechanisms unknown-vs-empty ingest-pipeline regression tests); develop alone collected 24995, this branch's merge collected 25022. Bumped 25050 -> 25060 on 2026-07-17 for tuning-capability-registry-coverage-20260716 (alias-resolution invariant tests and derived generator-coverage drift guards for the 9 newly-registered platforms); CI collected 25052 on the merge ref. Bumped 25060 -> 25080 on 2026-07-17 composing #1198's registry-coverage guards with #1176's provenance/hash export coverage (test_tuning_provenance_export.py + test_requested_config_hash.py); both branches independently bumped from 25050 (#1198: 25052 collected; #1176 pre-compose: 25062 collected); composed merge tree collects 25077. Bumped 25080 -> 25140 on 2026-07-17 for tuning-from-config-forwarding-sweep-20260716 (test_tuning_config_forwarding.py's registry-driven parametrized test, one case per registered platform adapter, verifying from_config forwards tuning_enabled/tuning_config/unified_tuning_configuration/tuning_source/tuning_source_file); timing_policy_check collected 25128. Bumped 25140 -> 25180 on 2026-07-18 for the todo-db-tracker local-SQLite spike (tests/unit/scripts/test_todo_db.py: 49 fast unit tests covering the enforced lifecycle invariants, archive import, and concurrency guards); develop collected 25128, this branch collects 25177. Bumped 25180 -> 25190 on 2026-07-18 composing the todo-db spike branch with develop's post-25128 growth: the branch alone collects 25177 (49 fast tracker tests; the 11 wrapper tests are medium-marked and collect 0 under -m fast), but the PR merge ref collects 25187. Bumped 25190 -> 25220 on 2026-07-18 for the post-CODEOWNERS-retirement merge queue, composed once so the queued branches carry byte-identical policy content instead of five conflicting bumps (#1142 seed-validation exclusion coverage +10; #1116 bot-finding sweep regressions +4; #1202 capability-registry/report regressions ~+8; #1206 object-schema loader regressions ~+5; #1114 branch-rename test moves +0): develop alone collects 25187; the queue's final merge ref is expected to collect ~25214.", "forbidden_marker_expressions": [ "resource_heavy", "stress", diff --git a/benchbox/core/expected_results/tpch_results.py b/benchbox/core/expected_results/tpch_results.py index c4b57840ed..e2fcbeb151 100644 --- a/benchbox/core/expected_results/tpch_results.py +++ b/benchbox/core/expected_results/tpch_results.py @@ -77,6 +77,26 @@ logger = logging.getLogger(__name__) +# Queries whose result cardinality is sensitive to the specific substitution +# parameters used (not just the scale factor) -- TPC-H's "answer-set boundary" +# queries. Only the pinned reference-seed answer set is guaranteed to match; +# a different seed produces valid-but-different query parameters that +# legitimately shift these queries' row counts, so an EXACT-mode mismatch on +# a non-reference-seed run is not a correctness regression. +# +# The bounded correctness gate (Makefile CORRECTNESS_GATE_QUERY_IDS, see +# docs/operations/release-guide.md "Q11/Q16/Q18/Q20 ... excluded for +# answer-set boundary sensitivity") already excludes this same set for the +# same reason. This constant is the production-code mirror that +# benchbox.core.validation.query_validation.QueryValidator consults so +# non-reference-seed runs (custom --seed, throughput streams whose derived +# seed is base_seed + stream_id*1000 + position) report an exclusion instead +# of a false EXACT-mode row-count failure. Not the same list object as the +# Makefile's (that one is test-only env-var plumbing feeding a single +# integration test, not importable production code) -- kept in sync by +# convention; a change to either should be reviewed against the other. +PARAMETER_SENSITIVE_QUERY_IDS: frozenset[str] = frozenset({"11", "16", "18", "20"}) + def get_tpch_expected_results(scale_factor: float = 1.0) -> BenchmarkExpectedResults | None: """Get expected results for TPC-H queries at a given scale factor. diff --git a/benchbox/core/tpch/power_test.py b/benchbox/core/tpch/power_test.py index d36bda32f3..d91c05caf4 100644 --- a/benchbox/core/tpch/power_test.py +++ b/benchbox/core/tpch/power_test.py @@ -18,6 +18,10 @@ from typing import Any, Optional from benchbox.core.plan_capture_phase import propagate_plan_capture_fields +from benchbox.core.validation.query_validation import ( + clear_reference_seed_context, + set_reference_seed_context, +) from benchbox.utils.clock import elapsed_seconds, mono_time @@ -138,6 +142,10 @@ def __init__( # Determine seed and validation mode based on user input and reference seed availability reference_seed = get_reference_seed(scale_factor) + # Stashed for run()'s per-query reference-seed context (see + # benchbox.core.validation.query_validation.set_reference_seed_context) -- + # avoids recomputing get_reference_seed() on every query. + self.reference_seed = reference_seed user_provided_seed = seed is not None actual_seed = seed # None = use qgen defaults mode (-d flag) actual_validation_mode = validation_mode or "exact" @@ -309,6 +317,15 @@ def run(self) -> TPCHPowerTestResult: if hasattr(self.connection, "set_query_context"): self.connection.set_query_context(query_id, stream_id=self.config.stream_id) + # Tell QueryValidator whether THIS query's parameters match the + # pinned reference seed (None/qgen-defaults counts as reference- + # equivalent, matching the __init__ seed-selection logic above) -- + # lets parameter-sensitive queries (Q11/16/18/20) be excluded + # instead of EXACT-failed when they don't. Cleared in the finally + # below regardless of outcome so it never leaks into unrelated + # validate_query_result() calls on this thread. + set_reference_seed_context(stream_seed is None or stream_seed == self.reference_seed) + cursor = self.connection.execute(query_text) # Check for validation failures from platform adapter @@ -337,6 +354,7 @@ def run(self) -> TPCHPowerTestResult: if hasattr(self.connection, "commit"): self.connection.commit() finally: + clear_reference_seed_context() # Capture labeled SQL for dry-run preview self.captured_items.append((label, query_text)) diff --git a/benchbox/core/tpch/throughput_test.py b/benchbox/core/tpch/throughput_test.py index 33de43d8f4..1f95ea32a3 100644 --- a/benchbox/core/tpch/throughput_test.py +++ b/benchbox/core/tpch/throughput_test.py @@ -22,6 +22,10 @@ from benchbox.core.plan_capture_phase import propagate_plan_capture_fields from benchbox.core.throughput.result import ThroughputResult, ThroughputStreamResult from benchbox.core.throughput.runner import StreamRunner +from benchbox.core.validation.query_validation import ( + clear_reference_seed_context, + set_reference_seed_context, +) from benchbox.utils.clock import elapsed_seconds, mono_time @@ -62,6 +66,21 @@ class TPCHThroughputTestConfig: TPCHThroughputStreamResult = ThroughputStreamResult +def _derive_query_seed(seed: int, stream_id: int, position: int) -> int: + """Per-query qgen seed for one throughput stream position. + + ``seed`` is the per-stream base seed StreamRunner hands to + ``_execute_stream`` (``config.base_seed + stream_id`` -- see + ``benchbox.core.throughput.runner``). The SINGLE definition of the + per-position formula: query generation (pre-generation pass and the + ``_resolve_query_text`` inline fallback) and the reference-seed + validation context in ``_execute_stream`` must all derive the seed + through this helper so validation can never silently desynchronize + from the SQL that was actually generated. + """ + return seed + stream_id * 1000 + position + + def _count_cursor_rows(cursor: Any) -> int: """Return a row count without importing benchbox.platforms from core. @@ -259,7 +278,7 @@ def _generate_one_stream(stream_id: int) -> tuple[int, list[Any]]: query_permutation = TPCHStreams.PERMUTATION_MATRIX[stream_id % len(TPCHStreams.PERMUTATION_MATRIX)] sql_list: list[Any] = [] for position, query_id in enumerate(query_permutation): - stream_seed = seed + stream_id * 1000 + position + stream_seed = _derive_query_seed(seed, stream_id, position) try: sql_list.append( self.benchmark.get_query( @@ -309,7 +328,7 @@ def _resolve_query_text( # Fall back to inline generation (_execute_stream called directly, # bypassing run()'s pre-generation pass). - stream_seed = seed + stream_id * 1000 + position + stream_seed = _derive_query_seed(seed, stream_id, position) return self.benchmark.get_query( query_id, seed=stream_seed, @@ -445,6 +464,18 @@ def _execute_stream( # in the background. See runner.py's module docstring. cancel_event = self._resolve_cancel_event(config, stream_id) + # Reference seed for this scale factor (None if scale_factor has no + # pinned reference, e.g. != 1.0) -- used below to tell QueryValidator + # whether EACH query's derived seed (seed + stream_id*1000 + position, + # same formula _resolve_query_text() uses to generate the SQL; not + # re-derived here, just re-read for validation-context purposes) + # matches the reference answer set, so parameter-sensitive queries + # (Q11/16/18/20) can be excluded instead of EXACT-failed when it + # doesn't. See benchbox.core.validation.query_validation. + from benchbox.core.tpch.benchmark import get_reference_seed + + reference_seed = get_reference_seed(config.scale_factor) + cancelled = False for position, query_id in enumerate(query_permutation): if self._cooperative_cancel_requested( @@ -474,6 +505,19 @@ def _execute_stream( if hasattr(connection, "set_query_context"): connection.set_query_context(query_id) + # Tell QueryValidator whether THIS query's derived seed + # (_derive_query_seed -- the same single definition query + # generation uses, so validation can never desynchronize + # from the generated SQL) matches the pinned reference + # seed -- lets parameter-sensitive queries (Q11/16/18/20) + # be excluded instead of EXACT-failed when the derivation + # doesn't happen to reproduce the reference answer set. + # Cleared in the finally below regardless of outcome so it + # never leaks into unrelated validate_query_result() calls + # on this thread. + stream_seed = _derive_query_seed(seed, stream_id, position) + set_reference_seed_context(stream_seed == reference_seed) + cursor = connection.execute(query_text) # Check for validation failures from platform adapter @@ -493,6 +537,7 @@ def _execute_stream( if hasattr(connection, "commit"): connection.commit() finally: + clear_reference_seed_context() # Capture labeled SQL for dry-run preview if hasattr(self, "captured_items"): self.captured_items.append((label, query_text)) diff --git a/benchbox/core/validation/query_validation.py b/benchbox/core/validation/query_validation.py index 88e1633414..7602532f49 100644 --- a/benchbox/core/validation/query_validation.py +++ b/benchbox/core/validation/query_validation.py @@ -12,15 +12,88 @@ from __future__ import annotations import logging +import threading # Import expected_results to trigger provider registration # This ensures TPC-H and TPC-DS providers are available when QueryValidator is instantiated import benchbox.core.expected_results # noqa: F401 from benchbox.core.expected_results.models import ValidationMode, ValidationResult from benchbox.core.expected_results.registry import get_registry +from benchbox.core.expected_results.tpch_results import ( + PARAMETER_SENSITIVE_QUERY_IDS as _TPCH_PARAMETER_SENSITIVE_QUERY_IDS, +) logger = logging.getLogger(__name__) +# Per-benchmark parameter-sensitive query-id sets consulted by the exclusion +# check in QueryValidator.validate_query_result(). Only TPC-H has one today +# (see tpch_results.PARAMETER_SENSITIVE_QUERY_IDS); benchmarks with no entry +# here get an empty set, so the exclusion never fires for them regardless of +# reference-seed context. +_PARAMETER_SENSITIVE_QUERY_IDS_BY_BENCHMARK: dict[str, frozenset[str]] = { + "tpch": _TPCH_PARAMETER_SENSITIVE_QUERY_IDS, + "tpc-h": _TPCH_PARAMETER_SENSITIVE_QUERY_IDS, +} + + +def get_parameter_sensitive_query_ids(benchmark_type: str) -> frozenset[str]: + """Return the parameter-sensitive query-id set for a benchmark, if any. + + Empty frozenset for benchmarks with no known parameter-sensitive queries + (e.g. TPC-DS today) -- the exclusion check in validate_query_result() + never fires for them. + """ + return _PARAMETER_SENSITIVE_QUERY_IDS_BY_BENCHMARK.get(benchmark_type.lower(), frozenset()) + + +# Thread-local reference-seed determination. Set by the TPC-H power/throughput +# drivers (benchbox.core.tpch.power_test, benchbox.core.tpch.throughput_test) +# immediately before executing a query, and read here to decide whether a +# parameter-sensitive query's EXACT-mode mismatch should be excluded instead +# of failed. Thread-local (not a plain module global) because throughput +# streams execute concurrently, one thread per stream (see +# benchbox.core.throughput.runner.StreamRunner) -- a single shared global +# would let one stream's context leak into a concurrently-running stream's +# validation call. +_reference_seed_state = threading.local() + + +def set_reference_seed_context(is_reference_seed: bool | None) -> None: + """Record, for the CURRENT THREAD, whether the query about to run through + QueryValidator is using the pinned TPC reference seed's substitution + parameters. + + This module never derives or compares seeds itself -- callers (the TPC-H + power/throughput drivers) compute the comparison against + benchbox.core.tpch.benchmark.get_reference_seed() and pass the result in. + + Values: + True: this query's actual seed matches the reference seed for its + scale factor (or the run used qgen defaults, which the TPC-H + drivers treat as reference-equivalent) -- exact validation + applies normally, with no exclusion. + False: this query is running under different substitution parameters + than the reference answer set -- get_parameter_sensitive_query_ids + entries are excluded from EXACT-mode failure rather than + compared. + None (the default/unset value): unknown -- preserves the pre-existing + behavior of always attempting EXACT validation. Benchmarks that + never call this function (TPC-DS, DataFrame validation, any other + QueryValidator caller) are therefore completely unaffected by the + exclusion below. + """ + _reference_seed_state.is_reference_seed = is_reference_seed + + +def get_reference_seed_context() -> bool | None: + """Return the current thread's reference-seed determination, if set.""" + return getattr(_reference_seed_state, "is_reference_seed", None) + + +def clear_reference_seed_context() -> None: + """Reset the current thread's reference-seed context to unset (None).""" + _reference_seed_state.is_reference_seed = None + class QueryValidator: """Validator for query execution results. @@ -132,6 +205,31 @@ def validate_query_result( # Benchmarks may use int keys but expected results use string keys query_id_normalized = self._normalize_query_id(benchmark_type, query_id) + # Parameter-sensitive exclusion (non-reference-seed runs only). TPC-H's + # answer-set-boundary queries (Q11/16/18/20 -- see + # get_parameter_sensitive_query_ids) only have a validated cardinality + # under the pinned reference seed's substitution parameters. When the + # caller has told us (via set_reference_seed_context) that the CURRENT + # query is running under different parameters, cardinality validation + # is inapplicable rather than a false failure -- mirrors the bounded + # correctness gate's existing exclusion policy (see + # docs/operations/release-guide.md: "Q11/Q16/Q18/Q20 ... excluded for + # answer-set boundary sensitivity"). Checked BEFORE the registry + # lookup so it applies regardless of what stream_id the caller passed. + # Reference-seed runs (context is True) and callers that never set the + # context (None, e.g. TPC-DS, DataFrame validation) are unaffected. + if get_reference_seed_context() is False and query_id_normalized in get_parameter_sensitive_query_ids( + benchmark_type + ): + return ValidationResult( + is_valid=True, + query_id=query_id_str, + expected_row_count=None, + actual_row_count=actual_row_count, + validation_mode=ValidationMode.SKIP, + warning_message=(f"Query '{query_id}' not validated (parameter-sensitive, non-reference params)."), + ) + # Get expected result from registry expected_result = self.registry.get_expected_result( benchmark_type, query_id_normalized, scale_factor, stream_id diff --git a/tests/integration/test_tpch_power_test.py b/tests/integration/test_tpch_power_test.py index 6187de92f4..f5f7824349 100644 --- a/tests/integration/test_tpch_power_test.py +++ b/tests/integration/test_tpch_power_test.py @@ -166,3 +166,159 @@ def render_query(query_id, **_kwargs): assert parts[0] == "Position" assert parts[2] == "Query" assert sample_sql == f"SELECT {parts[3]}" + + +class TestPowerTestReferenceSeedContext: + """tpch-throughput-seed-validation-fix w2/w3: TPCHPowerTest.run() must tell + QueryValidator, per query, whether the CURRENT stream's seed matches the + pinned reference seed for its scale factor -- see + benchbox.core.validation.query_validation.set_reference_seed_context().""" + + def test_reference_seed_context_true_for_qgen_defaults(self) -> None: + """No seed given -> qgen defaults mode, treated as reference-equivalent + (matches the pre-existing __init__ seed-selection assumption).""" + power_test = _make_power_test(scale_factor=1.0, seed=None) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.power_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + power_test.run() + + assert len(calls) == 22 + assert all(v is True for v in calls) + + def test_reference_seed_context_true_when_seed_matches_reference(self) -> None: + from benchbox.core.tpch.benchmark import TPCH_SF1_REFERENCE_SEED + + power_test = _make_power_test(scale_factor=1.0, seed=TPCH_SF1_REFERENCE_SEED) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.power_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + power_test.run() + + assert len(calls) == 22 + assert all(v is True for v in calls) + + def test_reference_seed_context_false_for_custom_seed(self) -> None: + """A custom seed that does not match the reference seed -> every query + in this stream is tagged non-reference (the exact w0 repro scenario: + seed=12345, SF=1.0).""" + power_test = _make_power_test(scale_factor=1.0, seed=12345) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.power_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + power_test.run() + + assert len(calls) == 22 + assert all(v is False for v in calls) + + def test_reference_seed_context_cleared_after_every_query(self) -> None: + power_test = _make_power_test(scale_factor=1.0, seed=12345) + + with patch("benchbox.core.tpch.power_test.clear_reference_seed_context") as mock_clear: + power_test.run() + + assert mock_clear.call_count == 22 + + def test_boundary_query_not_failed_with_custom_seed_at_sf1(self) -> None: + """End-to-end regression for the w0 defect: at SF=1.0 with a custom + (non-reference) seed, Q11/16/18/20 must not come back FAILED anymore. + + Routes through the REAL PlatformAdapterConnection + DuckDBAdapter + + QueryValidator stack (not a bare Mock connection, which would bypass + row-count validation entirely and silently not exercise the defect -- + see w1 notes on why the raw-Mock pattern elsewhere in this file never + caught this bug). Only the innermost raw DB cursor is mocked, forced + to return a row count that would fail an EXACT comparison against the + real SF=1 answer set. + """ + from benchbox.platforms.base.connection_wrappers import PlatformAdapterConnection + from benchbox.platforms.duckdb import DuckDBAdapter + + bench = Mock() + bench.get_query = Mock(return_value="SELECT 1") + + raw_connection = Mock() + + def raw_execute(query_text): + raw_result = Mock() + # A row count guaranteed to mismatch any real TPC-H SF=1 answer. + raw_result.fetchall = Mock(return_value=[(1,)] * 999) + return raw_result + + raw_connection.execute = Mock(side_effect=raw_execute) + + adapter = DuckDBAdapter() + connection = PlatformAdapterConnection(raw_connection, adapter) + connection.benchmark_type = "tpch" + connection.scale_factor = 1.0 + + power_test = TPCHPowerTest( + benchmark=bench, + connection=connection, + scale_factor=1.0, + seed=12345, + stream_id=0, + validation=True, + warm_up=False, + query_subset=["11", "16", "18", "20"], + ) + + result = power_test.run() + + assert result.success is True + assert result.queries_successful == 4 + assert all(qr["success"] for qr in result.query_results) + for qr in result.query_results: + assert qr.get("error") is None + + def test_boundary_query_still_fails_with_reference_seed_at_sf1(self) -> None: + """must_preserve: a REFERENCE-seed run keeps EXACT-match validation + for Q11/16/18/20 -- the same wrong-row-count cursor as above must + still fail the query when the seed matches the pinned reference.""" + from benchbox.core.tpch.benchmark import TPCH_SF1_REFERENCE_SEED + from benchbox.platforms.base.connection_wrappers import PlatformAdapterConnection + from benchbox.platforms.duckdb import DuckDBAdapter + + bench = Mock() + bench.get_query = Mock(return_value="SELECT 1") + + raw_connection = Mock() + + def raw_execute(query_text): + raw_result = Mock() + raw_result.fetchall = Mock(return_value=[(1,)] * 999) + return raw_result + + raw_connection.execute = Mock(side_effect=raw_execute) + + adapter = DuckDBAdapter() + connection = PlatformAdapterConnection(raw_connection, adapter) + connection.benchmark_type = "tpch" + connection.scale_factor = 1.0 + + power_test = TPCHPowerTest( + benchmark=bench, + connection=connection, + scale_factor=1.0, + seed=TPCH_SF1_REFERENCE_SEED, + stream_id=0, + validation=True, + validation_mode="exact", + warm_up=False, + query_subset=["11"], + ) + + result = power_test.run() + + assert result.success is False + assert result.queries_successful == 0 + assert not result.query_results[0]["success"] diff --git a/tests/integration/test_tpch_throughput_test.py b/tests/integration/test_tpch_throughput_test.py index 3bef864963..f7a9d50155 100644 --- a/tests/integration/test_tpch_throughput_test.py +++ b/tests/integration/test_tpch_throughput_test.py @@ -249,6 +249,141 @@ def test_run_success_gate_is_configurable(self) -> None: assert result.streams_successful == 1 +class TestThroughputReferenceSeedContext: + """tpch-throughput-seed-validation-fix w2/w3: _execute_stream() must tell + QueryValidator, per query, whether that query's derived seed + (seed + stream_id*1000 + position) matches the pinned reference seed for + its scale factor -- see + benchbox.core.validation.query_validation.set_reference_seed_context(). + Per-position offsetting means this is essentially only ever True for + stream 0's first query (position 0) -- see w1 notes.""" + + def test_reference_seed_context_true_only_for_stream0_position0(self) -> None: + from benchbox.core.tpch.benchmark import TPCH_SF1_REFERENCE_SEED + + benchmark = _make_benchmark_mock() + connections: list[Mock] = [] + factory = _make_connection_factory(connections) + + test = TPCHThroughputTest(benchmark=benchmark, connection_factory=factory, scale_factor=1.0, num_streams=1) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.throughput_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + test._execute_stream(stream_id=0, seed=TPCH_SF1_REFERENCE_SEED, config=test.config) + + assert len(calls) == 22 + assert calls[0] is True # position 0: seed + 0*1000 + 0 == reference seed + assert all(v is False for v in calls[1:]) # every other position diverges + + def test_reference_seed_context_false_for_default_base_seed(self) -> None: + """Default base_seed=42 never coincides with the reference seed at + any position -- the throughput driver's default config is always + non-reference (matches w0's live repro).""" + benchmark = _make_benchmark_mock() + connections: list[Mock] = [] + factory = _make_connection_factory(connections) + + test = TPCHThroughputTest(benchmark=benchmark, connection_factory=factory, scale_factor=1.0, num_streams=1) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.throughput_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + test._execute_stream(stream_id=0, seed=test.config.base_seed, config=test.config) + + assert len(calls) == 22 + assert all(v is False for v in calls) + + def test_reference_seed_context_cleared_after_every_query(self) -> None: + benchmark = _make_benchmark_mock() + connections: list[Mock] = [] + factory = _make_connection_factory(connections) + + test = TPCHThroughputTest(benchmark=benchmark, connection_factory=factory, scale_factor=1.0, num_streams=1) + + with patch("benchbox.core.tpch.throughput_test.clear_reference_seed_context") as mock_clear: + test._execute_stream(stream_id=0, seed=42, config=test.config) + + assert mock_clear.call_count == 22 + + def test_no_reference_seed_at_non_sf1_scale_factor(self) -> None: + """No pinned reference seed exists below SF=1.0 -- every query is + tagged non-reference regardless of the seed chosen.""" + from benchbox.core.tpch.benchmark import TPCH_SF1_REFERENCE_SEED + + benchmark = _make_benchmark_mock() + connections: list[Mock] = [] + factory = _make_connection_factory(connections) + + test = TPCHThroughputTest(benchmark=benchmark, connection_factory=factory, scale_factor=0.01, num_streams=1) + + calls: list[bool] = [] + with patch( + "benchbox.core.tpch.throughput_test.set_reference_seed_context", + side_effect=lambda v: calls.append(v), + ): + test._execute_stream(stream_id=0, seed=TPCH_SF1_REFERENCE_SEED, config=test.config) + + assert len(calls) == 22 + assert all(v is False for v in calls) + + def test_boundary_query_not_failed_on_stream1_with_default_seed_at_sf1(self) -> None: + """End-to-end regression for the w0 defect: at SF=1.0 with the + default base_seed, Q11/16/18/20 must not come back FAILED on ANY + stream (w0 found both stream 0 and stream 1 failing -- see w1 notes + on the separate, not-fixed-here set_query_context() stream_id + omission that widens EXACT-attempts to every stream; the parameter- + sensitive exclusion resolves it regardless of that omission). + + Routes through the REAL PlatformAdapterConnection + DuckDBAdapter + + QueryValidator stack via a connection_factory whose raw cursor + deliberately returns a row count (999) that mismatches every real + SF=1 answer -- so the OTHER (non-boundary) 18 queries are EXPECTED to + genuinely fail here (that's correct EXACT-mode behavior, not this + test's concern); only Q11/16/18/20 must be absent from the failures. + """ + from benchbox.platforms.base.connection_wrappers import PlatformAdapterConnection + from benchbox.platforms.duckdb import DuckDBAdapter + + benchmark = Mock() + benchmark.get_query = Mock(return_value="SELECT 1") + + def factory() -> PlatformAdapterConnection: + raw_connection = Mock() + + def raw_execute(query_text): + raw_result = Mock() + raw_result.fetchall = Mock(return_value=[(1,)] * 999) + return raw_result + + raw_connection.execute = Mock(side_effect=raw_execute) + raw_connection.close = Mock() + + adapter = DuckDBAdapter() + connection = PlatformAdapterConnection(raw_connection, adapter) + connection.benchmark_type = "tpch" + connection.scale_factor = 1.0 + return connection + + test = TPCHThroughputTest(benchmark=benchmark, connection_factory=factory, scale_factor=1.0, num_streams=2) + + # Per-stream seeds mirror StreamRunner's real derivation + # (config.base_seed + stream_id -- see benchbox/core/throughput/runner.py). + stream0 = test._execute_stream(stream_id=0, seed=test.config.base_seed + 0, config=test.config) + stream1 = test._execute_stream(stream_id=1, seed=test.config.base_seed + 1, config=test.config) + + boundary_ids = {11, 16, 18, 20} + for stream_result in (stream0, stream1): + failed_ids = {qr["query_id"] for qr in stream_result.query_results if not qr["success"]} + assert not (failed_ids & boundary_ids), ( + f"stream {stream_result.stream_id}: boundary queries wrongly failed: {failed_ids & boundary_ids}" + ) + + class TestCooperativeCancellation: """Cover the opt-in cooperative-cancel wiring in TPCHThroughputTest (w2).""" diff --git a/tests/unit/core/expected_results/test_validation_engine.py b/tests/unit/core/expected_results/test_validation_engine.py index 695643085b..b728f6b386 100644 --- a/tests/unit/core/expected_results/test_validation_engine.py +++ b/tests/unit/core/expected_results/test_validation_engine.py @@ -8,7 +8,14 @@ import pytest from benchbox.core.expected_results.models import ValidationMode -from benchbox.core.validation.query_validation import QueryValidator +from benchbox.core.expected_results.tpch_results import PARAMETER_SENSITIVE_QUERY_IDS +from benchbox.core.validation.query_validation import ( + QueryValidator, + clear_reference_seed_context, + get_parameter_sensitive_query_ids, + get_reference_seed_context, + set_reference_seed_context, +) pytestmark = [ pytest.mark.unit, @@ -96,3 +103,178 @@ def test_validate_tpcds_query(self): # expected_row_count is available but validation is skipped assert result.warning_message is not None assert "SKIP" in result.warning_message or "skip" in result.warning_message + + +class TestParameterSensitiveExclusion: + """Tests for the tpch-throughput-seed-validation-fix parameter-sensitive + exclusion: TPC-H's answer-set-boundary queries (Q11/16/18/20) should not + EXACT-fail when the caller (a TPC-H power/throughput driver) has recorded + that the current query is NOT running under the pinned reference seed's + substitution parameters (see set_reference_seed_context()). + """ + + @pytest.fixture(autouse=True) + def _reset_reference_seed_context(self): + """Guard every test in this class against thread-local context leakage. + + set_reference_seed_context()/clear_reference_seed_context() store + state on threading.local(), which persists across test functions + running on the same worker thread/process. Reset before AND after + each test so test order never matters. + """ + clear_reference_seed_context() + yield + clear_reference_seed_context() + + def test_parameter_sensitive_query_ids_constant(self): + """The TPC-H parameter-sensitive set matches the four documented + answer-set-boundary queries (mirrors the bounded correctness gate's + exclusion list -- docs/operations/release-guide.md).""" + assert frozenset({"11", "16", "18", "20"}) == PARAMETER_SENSITIVE_QUERY_IDS + + def test_get_parameter_sensitive_query_ids_tpch(self): + assert get_parameter_sensitive_query_ids("tpch") == PARAMETER_SENSITIVE_QUERY_IDS + assert get_parameter_sensitive_query_ids("tpc-h") == PARAMETER_SENSITIVE_QUERY_IDS + assert get_parameter_sensitive_query_ids("TPCH") == PARAMETER_SENSITIVE_QUERY_IDS + + def test_get_parameter_sensitive_query_ids_unknown_benchmark_is_empty(self): + """Benchmarks with no known parameter-sensitive queries (e.g. TPC-DS + today) get an empty set, so the exclusion can never fire for them.""" + assert get_parameter_sensitive_query_ids("tpcds") == frozenset() + assert get_parameter_sensitive_query_ids("unknown_benchmark") == frozenset() + + def test_context_default_is_none(self): + """Unset context (the default for every caller that never calls + set_reference_seed_context) reads back as None.""" + assert get_reference_seed_context() is None + + def test_context_set_get_clear_round_trip(self): + set_reference_seed_context(True) + assert get_reference_seed_context() is True + set_reference_seed_context(False) + assert get_reference_seed_context() is False + clear_reference_seed_context() + assert get_reference_seed_context() is None + + @pytest.mark.parametrize("query_id", sorted(PARAMETER_SENSITIVE_QUERY_IDS)) + def test_boundary_query_excluded_when_non_reference_seed(self, query_id): + """Q11/16/18/20 with a row count that would FAIL exact match are + excluded (SKIP, is_valid=True) once the context says non-reference.""" + set_reference_seed_context(False) + validator = QueryValidator() + result = validator.validate_query_result( + benchmark_type="tpch", + query_id=query_id, + actual_row_count=999_999_999, # deliberately implausible/wrong + scale_factor=1.0, + ) + assert result.is_valid + assert result.validation_mode == ValidationMode.SKIP + assert result.warning_message is not None + assert "parameter-sensitive" in result.warning_message + assert "non-reference params" in result.warning_message + + @pytest.mark.parametrize("query_id", sorted(PARAMETER_SENSITIVE_QUERY_IDS)) + def test_boundary_query_still_exact_validated_when_reference_seed(self, query_id): + """Reference-seed runs (context True) keep EXACT-match validation for + Q11/16/18/20 -- the exclusion must not fire (must_preserve).""" + set_reference_seed_context(True) + validator = QueryValidator() + result = validator.validate_query_result( + benchmark_type="tpch", + query_id=query_id, + actual_row_count=999_999_999, # deliberately implausible/wrong + scale_factor=1.0, + ) + assert not result.is_valid + assert result.validation_mode == ValidationMode.EXACT + assert result.error_message is not None + + @pytest.mark.parametrize("query_id", sorted(PARAMETER_SENSITIVE_QUERY_IDS)) + def test_boundary_query_still_exact_validated_when_context_unset(self, query_id): + """No caller ever set the context (e.g. a QueryValidator caller other + than the TPC-H power/throughput drivers) -- preserves the pre-existing + always-EXACT behavior exactly, so nothing regresses silently.""" + assert get_reference_seed_context() is None # sanity: truly unset + validator = QueryValidator() + result = validator.validate_query_result( + benchmark_type="tpch", + query_id=query_id, + actual_row_count=999_999_999, + scale_factor=1.0, + ) + assert not result.is_valid + assert result.validation_mode == ValidationMode.EXACT + + def test_non_boundary_query_not_excluded_when_non_reference_seed(self): + """The exclusion is scoped to the parameter-sensitive set only -- a + wrong row count on a non-excluded query (e.g. Q1) must still fail, + even with a non-reference-seed context (anti_pattern guard: no + unconditional exclusion, no tolerance widening).""" + set_reference_seed_context(False) + validator = QueryValidator() + result = validator.validate_query_result( + benchmark_type="tpch", + query_id="1", + actual_row_count=5, # TPC-H Q1 at SF=1 is 4, not 5 + scale_factor=1.0, + ) + assert not result.is_valid + assert result.validation_mode == ValidationMode.EXACT + + def test_boundary_query_skipped_regardless_of_count_when_non_reference_seed(self): + """The exclusion is deterministic: it fires BEFORE the registry + lookup, so under a non-reference context a parameter-sensitive query + is SKIPped no matter what count it returned -- even a count that + happens to equal the reference answer. This pins the always-SKIP + semantics (a coincidental match is NOT reported as an EXACT PASS, + because under different substitution parameters the reference + expectation is meaningless either way).""" + validator = QueryValidator() + + # Wrong count -> SKIP (not FAILED). + set_reference_seed_context(False) + wrong = validator.validate_query_result( + benchmark_type="tpch", + query_id="11", + actual_row_count=999_999_999, + scale_factor=1.0, + ) + assert wrong.is_valid + assert wrong.validation_mode == ValidationMode.SKIP + + # Reference-matching count -> still SKIP (not an EXACT PASS). Derive + # the reference expectation from the registry itself so this test + # never hardcodes an answer-set row count. + reference_expected = validator.registry.get_expected_result("tpch", "11", 1.0) + assert reference_expected is not None + reference_count = reference_expected.get_expected_count(1.0) + assert reference_count is not None + + set_reference_seed_context(False) + coincidental = validator.validate_query_result( + benchmark_type="tpch", + query_id="11", + actual_row_count=reference_count, + scale_factor=1.0, + ) + assert coincidental.is_valid + assert coincidental.validation_mode == ValidationMode.SKIP + assert coincidental.warning_message is not None + assert "parameter-sensitive" in coincidental.warning_message + + def test_tpcds_query_unaffected_by_reference_seed_context(self): + """TPC-DS never sets the reference-seed context, and has no entry in + the parameter-sensitive registry -- a non-reference-seed context must + not change its (pre-existing, SKIP-by-default) validation behavior.""" + set_reference_seed_context(False) + validator = QueryValidator() + result = validator.validate_query_result( + benchmark_type="tpcds", + query_id="1", + actual_row_count=101, + scale_factor=1.0, + ) + assert result.is_valid + assert result.validation_mode == ValidationMode.SKIP + assert "SKIP" in result.warning_message or "skip" in result.warning_message