Repository navigation
feat(tuning): validate requested tuning against single-source execution capabilities - #2919
joeharris76 wants to merge 14 commits into
Conversation
|
/oracle-review |
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for c5848e5e8af79e43529f2d3df34486be2948280d
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (196 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
One merge-blocking correctness defect: the new databend PRIMARY_KEYS rendered claim contradicts the adapter's PRIMARY KEY stripping, so validation silently accepts tuning that execution drops.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
|
/oracle-review |
1 similar comment
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: failure for 7c7d4445030407bc1ddb8256ab131d8ecb771684
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (208 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
D1 is fixed (Databend PK now not-rendered). New registry/test changes add unverified rendered-constraint claims (StarRocks UNIQUE/CHECK), omit Doris/Databend UNIQUE/CHECK so they warn instead of error, and the new parity test uses adapter-global signals so false rendered claims pass the gate.
Verified fixed: D1. Resolve their review threads.
Scope: files changed since head c5848e5e8af79e43529f2d3df34486be2948280d was reviewed.
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: failure for d1a6a7109b6b9f5f642ad5189f86cac9f8cd3b5a
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (234 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New registry entries correct the StarRocks/Doris/Databend UNIQUE/CHECK false-rendered claims, but the new parity harness retains adapter-global bypasses for the same constraint class, so the gate still cannot catch a recurrence.
Verified fixed: D2, D3, D4. Resolve their review threads.
Still open from earlier reviews
- Medium
tests/unit/core/tuning/test_capability_validator_d5.py:446-484: D5: Parity tests use adapter-global signals, cannot catch false rendered claims
Scope: files changed since head 7c7d4445030407bc1ddb8256ab131d8ecb771684 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 496c3a95d0c765bdc51cc7397da12abfbd29d0d3
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (234 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The new D5 test file adds real parity coverage but all three earlier gate-soundness defects persist verbatim, and the new file adds further bypasses and overbroad execution signals that let a false rendered/not-rendered registry claim pass. Fix the 3 carried defects plus the 4 new gate holes before merge.
Still open from earlier reviews
- Medium
tests/unit/core/tuning/test_capability_validator_d5.py:446-484: D5: Parity tests use adapter-global signals, cannot catch false rendered claims - High
tests/unit/core/tuning/test_capability_validator_d5.py:321-324: D6: Parity helper evidences UNIQUE/CHECK from DDL handoff alone - High
tests/unit/core/tuning/test_capability_validator_d5.py:595: D7: Noop-justification gate excused by adapter-global strip flag
Scope: files changed since head d1a6a7109b6b9f5f642ad5189f86cac9f8cd3b5a was reviewed.
|
/oracle-review |
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: failure for 67c7ee1a7ace1d71079b1dde596c4246d0097733
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (234 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The new D5 validator adds real gates (mysql covered, empty skip/override sets, citation existence) but its rendered/noop parity checks still use adapter-global signals, overbroad tokens, wrong-proxy adapters, circular absence justifications, and lifecycle-scoped absence scans, so false rendered claims and false noop excuses can still pass. 5 earlier defects remain open; 5 new blocking gate defects below must be fixed before merge.
Verified fixed: D8, D9. Resolve their review threads.
Still open from earlier reviews
- Medium
tests/unit/core/tuning/test_capability_validator_d5.py:446-484: D5: Parity tests use adapter-global signals, cannot catch false rendered claims - High
tests/unit/core/tuning/test_capability_validator_d5.py:321-324: D6: Parity helper evidences UNIQUE/CHECK from DDL handoff alone - High
tests/unit/core/tuning/test_capability_validator_d5.py:595: D7: Noop-justification gate excused by adapter-global strip flag - High
tests/unit/core/tuning/test_capability_validator_d5.py:288-289: D10: Generic .sql/.query attribute counts as statement execution - Medium
tests/unit/core/tuning/test_capability_validator_d5.py:602: D11: Noop justification content is never verified
Scope: files changed since head 496c3a95d0c765bdc51cc7397da12abfbd29d0d3 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 60b122e843ecd39b36d4d55fdf6f0ae7f44830cc
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (256 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
6 of 10 earlier validator-gate defects remain, plus 4 new registry/validator gaps; total open equals 10, so fixes rather than rework. Must fix false rendered UNIQUE/CHECK claims, missing-constraint warning downgrade, untested StarburstAdapter path, and narrow UNIQUE/CHECK vacuity proof before merge.
Verified fixed: D12, D13, D14, D15. Resolve their review threads.
Still open from earlier reviews
- Medium
tests/unit/core/tuning/test_capability_validator_d5.py:446-484: D5: Parity tests use adapter-global signals, cannot catch false rendered claims - High
tests/unit/core/tuning/test_capability_validator_d5.py:321-324: D6: Parity helper evidences UNIQUE/CHECK from DDL handoff alone - High
tests/unit/core/tuning/test_capability_validator_d5.py:595: D7: Noop-justification gate excused by adapter-global strip flag - High
tests/unit/core/tuning/test_capability_validator_d5.py:288-289: D10: Generic .sql/.query attribute counts as statement execution - Medium
tests/unit/core/tuning/test_capability_validator_d5.py:602: D11: Noop justification content is never verified - Medium
tests/unit/core/tuning/test_capability_validator_d5.py:155: D16: Type-evidence tokens are generic substrings
Scope: files changed since head 67c7ee1a7ace1d71079b1dde596c4246d0097733 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 553f4ef1c008abfe48dbee63ad38ab16b0225900
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (428 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Prior validator-harness defects D5-D20 are fixed by per-type evidence gates, stricter execution signals, machine-backed justifications, explicit SingleStore/Firebolt not-rendered entries, alias parity, and tree-wide vacuity proof. Two new blocking defects remain: the default-config fallback and the registry lookup do not normalize ':variant' platform strings, so 'duckdb:memory' keeps UNIQUE enabled and escapes the not-rendered error, leaving a wrong validation verdict and unverifiable tuned result.
Verified fixed: D5, D6, D7, D10, D11, D16, D17, D18, D19, D20. Resolve their review threads.
Defects without a review thread
- High
benchbox/core/tuning/capability_registry.py:1010: D21: Registry lookup does not normalize colon platform variants
resolve_platform_key lower-cases and maps aliases but never strips a ':variant' suffix, while callers pass values like 'duckdb:memory'. get_capability('duckdb:memory', ...) therefore returns None instead of the duckdb entry, so both the fallback filter and validate_for_platform_detailed fall through to the legacy compatibility path and miss the is_not_rendered_at_execution error for UNIQUE/CHECK and other not-rendered types. Normalize (split on ':' and trim) inside resolve_platform_key/get_capability so variants share the base platform rules.
Defects outside the diff count like the others: fix them before merge.
Not counted: defects in files this follow-up did not review
- High
benchbox/cli/tuning_runtime.py:17-20: Platform-variant suffix bypasses not-rendered fallback filtering
build_default_unified_config looks up the registry with the raw platform string (line 17) but normalizes only the later duckdb check with split(':',1)[0] (line 20). For 'duckdb:memory' get_capability returns None (resolve_platform_key keeps the ':memory' suffix, no such registry key), so unique_constraints stays enabled while 'duckdb' disables it. The test at tests/unit/cli/test_tuning_runtime.py:72-76 pins this divergence. The fallback for the variant therefore requests tuning the registry marks not-rendered on duckdb and validate_for_platform_detailed on the variant misses the not-rendered error, producing a wrong validation verdict and an unverifiable tuned result.
Scope: files changed since head 60b122e843ecd39b36d4d55fdf6f0ae7f44830cc was reviewed.
There was a problem hiding this comment.
oracle-verdict: pending for f64d49eac36283e91ad221d0d73b83c3026b6e5a
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (428 code lines exceed 150).
Scope: files changed since head 553f4ef1c008abfe48dbee63ad38ab16b0225900 was reviewed.
Reviewers not available
- muse: absent (invalid)
- sol: absent (error)
- luna: absent (error)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: success for 2724459d2200f1eb925d13a085ba61eda99e54cd
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (430 code lines exceed 150).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Can merge. D21 colon-variant normalization is fixed and tested. Reviewed the 5 changed files plus validation, sample-builder, and reconciliation interactions; new registry entries, default/sample builders disabling only not-rendered types, and validation rejecting them are consistent, with no provable benchmark-wrong, gate-weakening, or security defect in scope.
Verified fixed: D21. Resolve their review threads.
Scope: files changed since head 553f4ef1c008abfe48dbee63ad38ab16b0225900 was reviewed.
|
Changelog entry rewrite to the user-facing standard, ready to paste over the - Tuning validation now rejects tuning requests the platform cannot execute,
and platforms without tuning coverage error on unsupported types like
covered platforms. The registry corrects several platforms' coverage
(Postgres family, Doris, SingleStore, Databend, Starburst, MotherDuck,
DataFusion, and fabric-warehouse, which now has its own entry instead of
sharing Databricks'), and the sample and fallback configuration builders
only enable tuning the target platform supports. Layout tuning on
BigQuery, Redshift, Trino and Synapse now errors until their execution
support lands.This keeps every user-visible behavior and drops the registry internals ( Separate note: this PR still merge-conflicts with |
|
/oracle-review |
|
CI status note: the in-flight CI run was cancelled at 99% through fast tests (comment-policy on this head: 19 pass, 0 fail). I reran it on the current head just now. This repo cancels a PR's in-flight CI run whenever the PR description is edited, so please hold description edits and manual workflow cancels until this run lands; auto-merge is armed and will land it once CI and the oracle verdict are green. |
There was a problem hiding this comment.
oracle-verdict: success for 7d734a42d4a545d0ba5a276f6dfb26f94b364e5a
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (430 code lines exceed 150).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Only change since prior review is the added capability-validator test file; its discovery now covers benchbox.core and its evidence tokens separate partitioning from clustering, resolving D22/D23. No new merge-blocking correctness, validation-verdict, or security defects found in the added tests.
Verified fixed: D22, D23. Resolve their review threads.
Scope: files changed since head 71cddb8f2388ce2d8918e9e165dc6a706c1d7f67 was reviewed.
…on capabilities Implement the D-5 validator rule: a requested tuning type the registry marks as not rendered at execution is a validation error, and unknown platforms with registry coverage error on unsupported types like known platforms. Correct the registry for no-op surfaces across the Postgres family, Doris, SingleStore, Databend, Starburst, MotherDuck, DataFusion and fabric-warehouse, and make the sample and fallback configuration builders only enable tuning the target renders. Adds a registry-execution parity test that follows super and helper callees.
… not rendered Oracle review caught a false rendered claim: DatabendAdapter strips PRIMARY KEY clauses from every executed CREATE TABLE, so the _ddl claim silently accepted tuning execution drops. StarRocks foreign keys are stripped the same way. StarRocks primary keys stay rendered: the workload rewrite reads the tuned key columns to choose the key model.
…rity evidence Address oracle round-2 findings: databend primary keys and starrocks foreign keys were already corrected; starrocks unique/check move to not-rendered; doris and databend gain explicit unique/check not-rendered entries; the parity sweep now requires per-tuning-type evidence in reachable bodies instead of adapter-global signals.
The noop-justification gate accepted any not-rendered entry when the adapter stripped anything. Strips now justify only the matching constraint kind; every other not-rendered entry needs its own documented justification. Adds the databend materialized-views justification the tightened gate caught.
Map the mysql registry entry to its concrete mysql-wire representative instead of exempting it. Require per-type evidence for every rendered entry including unique and check constraints. Exclude directly returned calls from execution credit. Add rendered-override staleness coverage. Verify every noop justification cites a resolvable source file and machine-check absence claims against adapter lifecycles.
…o registry entries
…CK claims No in-tree benchmark consumes UNIQUE/CHECK tuning flags, so every rendered UNIQUE/CHECK registry entry validated clean while execution rendered nothing. Mark them not-rendered tree-wide with vacuity-backed justifications, and add the missing constraint entries (firebolt, questdb, pg-duckdb, pg-mooncake, ClickHouse FK/CHECK, BigQuery UNIQUE, trino/presto PK/FK) so unrendered requests error instead of warning. Tighten the validator gates to match: rendered UNIQUE/CHECK require lifecycle type tokens, type tokens match on word boundaries, .sql/.query count as execution only on connection-like receivers with statement arguments, kind-matched strips only, every noop justification machine-backed, StarburstAdapter covered by both parity gates, and the vacuity proof enumerates every in-tree benchmark plus a layout-flag invariance companion. Shipped, example, and generated Snowflake/DuckDB templates disable the flags they cannot render.
…and coverage tests Fallback and auto resolution build their config through build_default_unified_config, which disables every constraint type the capability registry marks not-rendered. UNIQUE/CHECK are not-rendered on every platform, so postgres fallback/auto no longer request CHECK: expect CHECK/UNIQUE disabled with PK still enabled, under a name that no longer claims postgres requests CHECK. The coverage test loads a rendered-only config on the clean-load path and pins that a bare-default load on duckdb raises under the requested-but-not-rendered gate.
…ules
resolve_platform_key now strips any ':variant' suffix before lowercasing
and alias mapping, so get_capability('duckdb:memory', ...) returns the
duckdb entry and validate_for_platform_detailed reports not-rendered
errors for UNIQUE/CHECK instead of falling through to the legacy path.
build_default_unified_config looks up the registry with the normalized
key, and the duckdb:memory test asserts shared base rules.
Benchmark discovery for the vacuity proofs walked only top-level benchbox modules, so benchmarks defined under benchbox.core and not re-exported (ai_primitives, joinorder_synthetic) were never checked for UNIQUE/CHECK or layout-flag rendering. Discovery now also walks benchbox.core, fails on any module that cannot be imported, and asserts that the core benchmarks stay discovered. Partitioning evidence tokens included 'cluster by', so a clustering-only implementation counted as partitioning. Those tokens now evidence clustering only. Snowflake's partitioning entry, which really is issued as ALTER TABLE ... CLUSTER BY, is attested by an explicit justification instead.
7d734a4 to
3e2cade
Compare
There was a problem hiding this comment.
oracle-verdict: success for 3e2cade18e4101ec242feaa0a64b133af5a6c093
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (430 code lines exceed 150).
Decision: SHIP, carried from head 7d734a42d4a545d0ba5a276f6dfb26f94b364e5a.
The patch is unchanged since head 7d734a42d4a545d0ba5a276f6dfb26f94b364e5a was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
…lity-validator # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
oracle-verdict: success for fc828ba3919976c33747fdfd2b9b9b1d1f16d991
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (430 code lines exceed 150).
Decision: SHIP, carried from head 3e2cade18e4101ec242feaa0a64b133af5a6c093.
The patch is unchanged since head 3e2cade18e4101ec242feaa0a64b133af5a6c093 was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
This branch was successfully deployed
1 participant
Summary
Tuning validation now enforces what execution actually renders. A requested
tuning type the capability registry marks as not rendered at execution is a
validation error (
requested-but-not-rendered), and unknown platforms withregistry coverage error on unsupported types like known platforms. The
registry, the validator, and reconciliation all read the same
rendered_via/mechanism_idsource through one shared helper.Soundness review
This change touches trust paths (tuning validation, capability registry,
reconciliation, sample/fallback builders). Requesting soundness review of the
current head before merge. It merges once the oracle approves the current head.
Soundness questions for the reviewer:
non-constraint types). Unaudited platforms without registry coverage keep
warn-first only via the constraint exception; layout requests on them now
error. Confirm this matches the single-source execution intent.
fabric-warehouselost its Databricks alias for a verified dedicated entry(T-SQL ignores tuned partitioning).
template_evidencelookups and thetuning validateplatform key now resolve tofabric-warehouse.the shared tuning-aware DDL path (verified empirically per dialect), so the
registry keeps
_ddlfor PK/FK and only DataFusion moves to_none.UNIQUE/CHECK are now
_noneon every platform: no in-tree benchmarkconsumes those flags (pinned tree-wide by
test_unique_check_rendering_is_vacuous_in_tree), so requesting them rendered
nothing. Shipped and example DuckDB/Snowflake templates now disable those
flags, and the snowflake template generator header matches.
entries (previously missing entries only warned); trino/presto gained
PK-not-rendered plus FK-rendered entries matching the shared
presto-trino base (PK stripped, FK preserved); ClickHouse gained explicit
FK/CHECK entries; BigQuery gained an explicit UNIQUE entry.
tokens (no DDL-handoff-only evidence), type tokens match on word
boundaries,
.sql/.querycount as execution only onconnection-like receivers with statement arguments, the adapter-global
strip signal is gone (kind-matched strips only), every noop justification
is machine-backed (strip/uncalled/returns-empty/absence/behavioral/
generator-quiet/vacuity/layout-quiet), StarburstAdapter runs both parity
gates against the trino entries, and the UNIQUE/CHECK vacuity proof plus a
new layout-flag-invariance proof enumerate every in-tree benchmark.
Behavior changes (all intended under single-source execution validation)
their render items land in the next batch. Do not cut a release on this
head without those items or an owner override.
tuning initsamples and fallback configs only enable tuning the targetrenders (e.g. no Bloom filters on Databricks samples, no constraints on
DataFusion/MotherDuck/Doris fallbacks).
errors instead of warnings where the registry marks the type not-rendered.
:variantplatform strings (e.g.duckdb:memory) resolve to base-platform rules in the registry lookup and the default-config fallback, so variants share not-rendered errors with their base platform.requested PK/FK error where execution drops them (firebolt, questdb,
trino/presto PK, ClickHouse FK). The frozen compat baseline gains bigquery
UNIQUE plus ClickHouse FK/CHECK entries (explicit not-rendered entries join
the compat set, matching existing entries).
ClickHouse UNIQUE error, starrocks silence, drift-set growth,
reconciliation drop reasons/notes, fallback builder expectations).
Verification
uv run -- python -m pytest -m fast -q: 38,022 passed, 26 skipped (2pre-existing-behavior tests updated to the new intended semantics, no
assertion relaxed).
uv run -- ruff check .,ruff format --check .,ty check: clean.make comment-policy-check,make agent-write-preflight: pass.make pr-preflight: pass (see CI).