fix(validation): exclude TPC-H Q11/16/18/20 from EXACT row-count checks under non-reference seeds#1142
Merged
Merged
Conversation
…ation-fix w0 confirms the Q11/16/18/20 EXACT-validation defect at SF=1.0 (SF 0.01 does not reproduce -- expected results only load at SF=1.0); w1 pins the seam to query_validation.py/registry.py/tpch_results.py/power_test.py/throughput_test.py and documents why a thread-local context channel (not a new function param) is required to stay within scope_limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ks under non-reference seeds
QueryValidator EXACT-validates every TPC-H query at stream 0 against the
pinned reference-seed answer set regardless of what seed actually produced
the query's substitution parameters. Q11/16/18/20 (TPC-H's answer-set-
boundary queries, already excluded from the bounded correctness gate for the
same reason) legitimately return different row counts under a different
seed, so a custom --seed or a throughput stream's derived seed
(base_seed + stream_id*1000 + position) made them fail every run.
Adds a thread-local reference-seed context
(benchbox.core.validation.query_validation.set_reference_seed_context) that
the TPC-H power/throughput drivers set immediately before each query
execute(), computed from the seed actually in effect vs
get_reference_seed(scale_factor). QueryValidator excludes the four
parameter-sensitive queries (new PARAMETER_SENSITIVE_QUERY_IDS constant in
expected_results/tpch_results.py) only when that context is explicitly
non-reference; reference-seed runs and any caller that never sets the
context (TPC-DS, DataFrame validation) keep exact-match behavior unchanged.
A context channel (not a new execute_query() parameter) keeps the change
inside benchbox/core/{tpch,validation,expected_results}/ -- threading an
explicit argument would have required touching every platform adapter's
execute_query() signature.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ep verified) make uat-sweep CONFIG=tests/uat/configs/uat-throughput-duckdb-nightly.yaml (SF=1, 3 streams, non-reference seed 20260709) passes clean: validator_status=clean, 0 errors, Throughput@Size=265178.57, Q11/16/18/20 SUCCESS on all 3 streams -- the cell this fix was written to unblock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on exclusion - Rewrite the mislabeled always-SKIP unit test: under a non-reference context a parameter-sensitive query is SKIPped deterministically (the exclusion fires before the registry lookup), so pin BOTH the wrong-count and the coincidentally-reference-matching-count cases as SKIP. - Extract _derive_query_seed() in throughput_test.py as the single definition of seed + stream_id*1000 + position, used by pre-generation, the inline generation fallback, and the reference-seed validation context so they can never silently desynchronize. - Restore test_validate_tpcds_query's original final warning-message assertion that the class splice had dropped. - Use StreamRunner's real per-stream seed derivation (base_seed + stream_id) in the two-stream integration regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joeharris76
changed the title
joeharris76
enabled auto-merge (squash)
July 17, 2026 02:38
14 tasks
Resolves the modify/delete conflict on tpch-throughput-seed-validation-fix.yaml: this branch already completed and archived the TODO to DONE/ (405075d), which develop's tip hadn't seen yet since this PR hasn't merged. Kept our deletion. Catches this branch up with develop so CI can run again.
…d-validation tests Merge ref collects 25197 (develop alone 25187 + 10 fast tests from this branch's PARAMETER_SENSITIVE_QUERY_IDS / reference-seed-context coverage). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018y41mPzRqiLqvZLZCxwS6j
Replaces this branch's solo 25190->25200 bump with the byte-identical composed 25190->25220 policy content shared by every queued open PR, so successive merges to develop cannot conflict on this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018y41mPzRqiLqvZLZCxwS6j
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 18, 2026
…#1179) (#1202) * fix(tuning): pg-duckdb registry missing SORTING capability entry PgDuckDBDDLGenerator inherits PostgreSQLDDLGenerator.SUPPORTED_TUNING_TYPES, which includes sorting -- a tuned SORTING config is processed (as a CLUSTER-index pair via postgresql.py's shared clustering path, then the CLUSTER statement filtered out by pg_duckdb.py's override), not rejected. The registry entry only covered PARTITIONING/CLUSTERING, so get_capability("pg-duckdb", SORTING) returned None ("not compatible") instead of rendered_via="none" ("compatible, but execution renders nothing") -- and benchbox tuning platforms silently omitted the row. * fix(uat): gate-check timezone bug for naive Docker lifecycle timestamps release_gate_ordering_violations used timestamp.astimezone() to attach a timezone to append_lifecycle_log()'s naive Docker timestamps, but astimezone() interprets a naive datetime as being in the *checker process's* current local timezone, not the producer's. A boundary completed at 2026-05-30T01:00:00-07:00 (PDT) with a genuinely-later Docker naive timestamp of 2026-05-30T02:00:00 would get +00:00 attached instead of -07:00 when the checker runs under UTC, making it appear hours before the boundary and firing a false ordering violation. Both timestamps come from the same host/sweep run, so replace() with the boundary's own tzinfo is correct where astimezone() was not. * fix(todo): align w2 with ratified nullability policy; fix 3 scope/dependency gaps - uat-clickhouse-server-loader-type-coverage.yaml: w2's notes still told the implementer to use type defaults for genuinely non-Nullable columns when source semantics permit, directly contradicting the now-ratified open_questions[0] decision (no coercion, ever; fail loudly on empty-into-NOT-NULL). Rewrote w2 to match. Also added the ratified decision item (uat-clickhouse-loader-empty-value-nullability) to deps.needs so the DAG reflects the real prerequisite, not just prose. - tpch-throughput-parameter-aware-rowcounts.yaml: scope_limit restricted to tests/uat/throughput.py + tests/uat/test_throughput.py, but the real validation seam is benchbox/core/tpch + expected_results + validation (per PR #1142) and the parent's own anti_patterns explicitly forbid special-casing in tests/uat. Expanded scope_limit to the real seam. - uat-throughput-result-path-manifest.yaml: scope_limit omitted tests/uat/runner.py, whose `official` branch calls resolve_official_result_path directly without passing/parsing stdout_text -- if w1 picks the quiet-stdout-line contract, runner.py's official branch must change too, or w2 cannot retire the glob within scope. Added runner.py + its test file, and documented the handoff in w2's notes. * fix(uat): append_lifecycle_log must write offset-aware timestamps The #1179 fix (this same branch) patched release_gate_ordering_violations to reuse the boundary's own offset for naive Docker timestamps, but that degrades across a DST transition mid-sweep: a boundary and a later event can genuinely carry different UTC offsets (e.g. PDT -07:00 -> PST -08:00 after fall-back), and reusing the boundary's fixed offset for the later event misreads its wall-clock time, producing a false ordering violation. Fix at the real source instead: append_lifecycle_log() now writes datetime.now().astimezone() (offset-aware) rather than plain datetime.now() (naive), so each Docker lifecycle event carries its own real offset and survives a DST transition correctly. The boundary-offset substitution in release_gate_ordering_violations remains as a documented best-effort fallback for uat_lifecycle.log files written by an older BenchBox version that never recorded an offset at all. * fix(ci): bump fast-lane test-count ceiling for pr-review-followup-sweep-15 FAST_LANE_VIOLATION: fast lane count 25085 exceeds limit 25080. Shared repo-wide fast-lane budget outpaced again by cumulative batch merges. Bump max_fast_tests 25080 -> 25100 per maintainer direction. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements TODO
tpch-throughput-seed-validation-fix(uat-hardening batch, #1138): TPC-H's answer-set-boundary queries Q11/16/18/20 no longer fail EXACT row-count validation when a run's substitution parameters come from a non-reference seed — the single defect that kept every nightly throughput UAT cell red and forcedcontinue-on-error: trueon the nightly sweep.PARAMETER_SENSITIVE_QUERY_IDS = frozenset({"11", "16", "18", "20"})inbenchbox/core/expected_results/tpch_results.py— the production-code mirror of the bounded correctness gate's existing exclusion policy (docs/operations/release-guide.md).benchbox/core/validation/query_validation.py(set_reference_seed_context/get_reference_seed_context/clear_reference_seed_context).QueryValidator.validate_query_result()excludes the four queries (SKIP, reason recorded as "not validated (parameter-sensitive, non-reference params)") only when the context is explicitly non-reference. Unset context preserves the pre-existing always-EXACT behavior, so TPC-DS/DataFrame validation and every other caller are untouched.benchbox/core/tpch/power_test.pyandthroughput_test.pyset/clear the context around each query execute, computed from the seed actually in effect vsget_reference_seed(scale_factor). Throughput derives per-query seeds through a new single-definition_derive_query_seed()helper (same formula as before:seed + stream_id*1000 + position), so validation can never desynchronize from the generated SQL.Verification
295b645): SF=1--seed 12345power run fails exactly Q11/16/18/20; throughput (2 streams) fails exactly those 4 × both streams. Fixed end-to-end after this change.tests/unit/core/9993 passed;tests/unit/core/expected_results/122 passed; power+throughput integration files 28 passed;make lint,lint-imports,lint-markers,tyclean.make uat-sweep CONFIG=tests/uat/configs/uat-throughput-duckdb-nightly.yaml(SF=1, 3 streams, non-reference seed) — all phases exit 0, cellpassed/submittable,validator_status=clean(100% clean rate), Throughput@Size = 265178.57, Q11/16/18/20 SUCCESS on all 3 streams.ac294f7.Code Owner review required
benchbox/core/expected_results/**(a CODEOWNERS soundness-critical path), so auto-merge is intentionally not enabled — it awaits Code Owner review before merge.Follow-ups (flagged, not in this PR)
set_query_context(query_id)omitsstream_id=(every stream treated as stream 0 for validation scoping).🤖 Generated with Claude Code