Repository navigation
fix(tuning): verify DuckDB primary, unique and foreign keys against duckdb_constraints() - #2830
Conversation
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-review-shadow: failure for b0aaffb3334a54201bed203036728811a08ceae7
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (372 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
One blocking false-verification defect: constraint normalization can equate distinct physical columns. Static review used the supplied diff and head files because the base commit is unavailable locally. No tests were run.
There was a problem hiding this comment.
oracle-review-shadow: failure for 972fab1ab0e2bb59cc1a913ca945b204466c1c70
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (424 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
D1 is fixed. Two new blocking defects remain: dollar-quoted literals can bypass CHECK corroboration, and catalog filtering falsely reports constraints on supported quoted table names as absent. Review completed through static inspection only.
Verified fixed: D1. Resolve their review threads.
Scope: files changed since head b0aaffb3334a54201bed203036728811a08ceae7 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 020388612c320f38d854cf72e502095abfc10a74
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (441 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
Both earlier defects are fixed, but two remaining paths can falsely certify missing constraints. Fix these before merge. Review used static inspection only; no tests or network operations were run.
Verified fixed: D2, D3. Resolve their review threads.
Scope: files changed since head 972fab1ab0e2bb59cc1a913ca945b204466c1c70 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for af5e060eaf51db25f2d2db4b09832023950ff4e8
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (465 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
Both earlier defects are fixed. One remaining literal-scanning bypass can falsely certify a ClickHouse statement whose CHECK constraint was never corroborated. Fix it before merge. Review used static inspection only.
Verified fixed: D4, D5. Resolve their review threads.
Scope: files changed since head 020388612c320f38d854cf72e502095abfc10a74 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 739d46bead47c9e704eb53b05cdd79633250f1af
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (466 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
D6 is fixed, but two remaining lexical gaps can omit CHECK constraints and incorrectly grant applied_verified. Both require fixes before merge. Review was static; no tests or network operations were run.
Verified fixed: D6. Resolve their review threads.
Scope: files changed since head af5e060eaf51db25f2d2db4b09832023950ff4e8 was reviewed.
There was a problem hiding this comment.
oracle-verdict: pending for 536edf99ec527eadb492d533ed10b3895f280ece
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (466 code lines exceed 150; touches 2 areas).
Scope: files changed since head 739d46bead47c9e704eb53b05cdd79633250f1af was reviewed.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
- muse: absent (invalid)
- 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)
There was a problem hiding this comment.
oracle-verdict: failure for 536edf99ec527eadb492d533ed10b3895f280ece
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (466 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New constraint corroboration is sound fail-closed except one over-broad guard: the block-comment pattern matches every closed /.../ comment, forcing UNVERIFIABLE on statements the PR's own tests require to stay verifiable. Narrow the guard to unmaskable (nested/unclosed) comments or update the tests.
Verified fixed: D7, D8. Resolve their review threads.
Scope: files changed since head 739d46bead47c9e704eb53b05cdd79633250f1af was reviewed.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
There was a problem hiding this comment.
oracle-verdict: success for c4904ae281e7fda24e3d949db7d9e802b27d6ba8
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (467 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
SHIP: the new constraint parser/matcher is fail-closed (unknown CHECK/named/referential-action/qualified/dialect-quoted/unmaskable-literal shapes return UNVERIFIABLE), FK matching requires child columns plus referenced table and columns, incomplete catalog FK facts yield UNVERIFIABLE rather than corroboration, and platforms without a constraint source stay UNVERIFIABLE so prior sort-key-only verification cannot silently pass. Prior defect D9 was never a defect: the guard detects a nested block-comment opener, not every closed comment. No new merge-blocking defects found in the changed files.
Verified fixed: D9. Resolve their review threads.
Scope: files changed since head 536edf99ec527eadb492d533ed10b3895f280ece was reviewed.
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: pending for d048235e9f49302ee336f38e96a54c01156e9b78
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (467 code lines exceed 150; touches 2 areas).
Scope: files changed since head c4904ae281e7fda24e3d949db7d9e802b27d6ba8 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)
There was a problem hiding this comment.
oracle-verdict: success for d048235e9f49302ee336f38e96a54c01156e9b78
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (467 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New DuckDB constraint introspection is fail-closed: bounded catalog reads, current-schema filtering with multi-schema ambiguity degrading to error, ASCII-fold physical identifier matching, CHECK/unparseable statements staying unverifiable, and truncation degrading the receipt. No false-corroboration, gate-weakening, or injection path found in the changed files or their interaction with the constraint matcher.
Scope: files changed since head c4904ae281e7fda24e3d949db7d9e802b27d6ba8 was reviewed.
…uckdb_constraints() Constraint-bearing CREATE TABLE statements had no corroboration rule, so every tuned DuckDB run that requested keys, including the shipped TPC-H template, stayed applied_unverified. The classifier now reads PRIMARY KEY, UNIQUE and FOREIGN KEY clauses from the column list of a CREATE TABLE, table-level and column-level, and emits one constraint intent per clause with a constraint_type. Keys match a catalog fact of the same type on equal columns in order; a foreign key also needs the same referenced table and referenced columns. Anything in the column list that is not fully parsed (CHECK, named CONSTRAINT, referential actions, REFERENCES without columns) keeps the whole statement unverifiable. The DuckDB introspector reads duckdb_constraints() in a bounded query filtered to the three key types, the current database and schema, and the ledger's tables. A foreign-key fact without referenced columns is unverifiable. Introspectors that do not read constraints (every other platform) leave constraint entries unverifiable, including when the same statement carries a corroborated sort, partition or cluster key. DuckDB auto and tuned-fallback configurations no longer enable CHECK constraints, since no catalog read can confirm them.
Constraint facts and parsed constraint clauses went through
normalize_columns and normalize_identifier, which strip whitespace and
surrounding quote characters. A catalog key on the column " a " therefore
became ('a',) and could corroborate a requested key on a, for example when
CREATE TABLE IF NOT EXISTS ran against an existing table. The same applied
to unique and foreign-key columns and to table names.
Constraint identifiers now compare the way DuckDB resolves them: ASCII
letters fold to lower case and everything else, including whitespace inside
a double-quoted identifier, is kept. Catalog names are folded without
stripping. A table, column or referenced name that is not one bare or one
double-quoted identifier (schema-qualified, backtick- or bracket-quoted,
several tokens, an empty or dangling list item) keeps the statement
unverifiable. Index and sort-key matching is unchanged.
…t tables by physical name The literal and comment scanner does not mask DuckDB dollar-quoted strings ($$...$$, $tag$...$tag$), escape strings (E'...') or nested block comments. A parenthesis inside one could end the parsed column list early, so a CHECK after it was never seen and an existing key could corroborate. A CREATE TABLE with a column list that contains any of these forms now yields no constraint intent and stays unverifiable as a whole. The DuckDB constraint read filtered on ledger_tables(), whose table regex stops at whitespace and does not unescape doubled quotes, so the constraints of a table such as "my t" or "q""t" were never read and came back absent. The read now takes its table names from the constraint intents themselves (constraint_tables), which use the same physical-identifier parser as the matching. Index reads and index and sort-key matching are unchanged.
…nt tables The literal scanner treats `[` and backticks as identifier quotes, but DuckDB reads `[` as list syntax and a backtick as an operator. A list default such as `[']', ')']` could end the scanner's bracket quote inside a string, expose a parenthesis and hide a CHECK clause, so an existing key corroborated a statement whose CHECK was never verified. A CREATE TABLE that carries constraint text next to either character is now unverifiable. Constraint intents carry unqualified table names, and the catalog was read in the schema current at introspection time. After `SET schema`, `SET search_path` or `USE`, a same-named table elsewhere could corroborate the target. The DuckDB introspector now degrades the state when a constrained table name exists in more than one database schema, including temporary tables, since the catalog cannot show which table the statement changed.
The literal scanner ends a single-quoted string at the next quote, but some engines read backslash-quote as an escaped quote. A default such as '\')\'' could expose a parenthesis and hide a CHECK constraint while a surviving sort key verified alone. A CREATE TABLE holding a backslash directly before a single quote is now unverifiable as a whole; the check fires only on that pair, so ordinary backslashes still parse.
…SCII dollar tags The literal scanner ends strings and quoted identifiers at the next quote, but ClickHouse reads a backslash before a double quote inside a quoted identifier as an escaped quote, and DuckDB accepts non-ASCII dollar-quote tags. Either shape could expose a parenthesis and hide a CHECK constraint while a surviving sort key or primary key verified alone. A CREATE TABLE holding a backslash directly before either quote style, or a dollar-quote delimiter with a non-ASCII tag, is now unverifiable as a whole; the checks fire only on those shapes, so ordinary backslashes and plain quoted identifiers still parse.
d048235 to
0a60401
Compare
There was a problem hiding this comment.
oracle-verdict: success for 0a60401d1bbcc422517fbc84ee5c8b5bab6dfb0f
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (467 code lines exceed 150; touches 2 areas).
Decision: SHIP, carried from head d048235e9f49302ee336f38e96a54c01156e9b78.
The patch is unchanged since head d048235e9f49302ee336f38e96a54c01156e9b78 was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
… corroboration Since #2830 the shipped DuckDB TPC-H template verifies constraints, so the tuned run prints applied_verified. Updates the stale expectation; the summary-print assertions are unchanged.
… summary (#2863) * feat(tuning): write local results unanonymized and print verification summary Local result files now keep receipt statements, reasons and errors so users can see why tuning did not verify, with connection-username redaction retained. Publish, public export and submission keep full anonymization: outward paths re-export anonymized or refuse unanonymized tuning diagnostics, and both submission validation and the artifact privacy check reject them. Every run prints status, counts by verdict, top reasons and dropped intents. * fix(tuning): close privacy gaps in anonymized export, redacted publish, privacy gate Strip receipt entry errors during anonymization so anonymized exports pass the tuning detector. Refuse redacted publish when a companion cannot be re-exported redacted, and report the actually-published companion count. Fail the artifact privacy scan closed when the tuning detector cannot be imported. * fix(tuning): scan publish companions before verbatim return * fix(tuning): record original source on redacted publish, gate verbatim on plans redaction Redacted publishes record the original result path instead of the deleted scratch directory, so publication history resolves and re-publishes dedupe. The verbatim fast path now also scans plans companions for unredacted explain output and operator metadata, re-exporting when dirty. * fix(tuning): validate plans companion in submission, gate verbatim publish on export anonymized - In validate_bundles, iterate over companion files to find .plans.json companions and validate them with unanonymized_plans_findings(), reporting any violations on the validation result vr. - In _redacted_publish_source, refuse verbatim return if payload.get("export", {}).get("anonymized") is not True. * fix(tuning): validate tuning companion and record companion count before scratch cleanup * fix: verify anonymized results before publishing * fix: preserve clean publish companions * fix: refuse dirty tuning companions during publish * fix(test): expect applied_verified in DuckDB summary after constraint corroboration Since #2830 the shipped DuckDB TPC-H template verifies constraints, so the tuned run prints applied_verified. Updates the stale expectation; the summary-print assertions are unchanged.
This branch was successfully deployed
1 participant
Description
Tuned DuckDB runs that requested primary or foreign keys could never reach
applied_verified: every constraint-bearingCREATE TABLEhad no corroboration rule, so the shipped DuckDB TPC-H template always reportedapplied_unverified. This implements the ADR-001 addendum of 2026-10-04, "which constraints are catalog-verifiable".benchbox/core/tuning/introspection.py). Constraint clauses are read only from the column list of aCREATE TABLE(the parenthesized group after the table name), after string literals and comments are blanked. Table-levelPRIMARY KEY (cols),UNIQUE (cols)andFOREIGN KEY (cols) REFERENCES t (cols), and the column-levelPRIMARY KEY,UNIQUEandREFERENCES t (cols)forms the SSB and read-primitives schemas use, each become aconstraintintent with aconstraint_type. Foreign-key intents carryreferenced_tableandreferenced_columns. Any other constraint text in the column list keeps the whole statementunverifiable:CHECK, a namedCONSTRAINT,ON DELETE/ON UPDATE/MATCH,REFERENCESwithout a column list, trailing clause text, an empty key, an unbalanced list, or a second SQL statement after the column list in the same ledger entry.constraintfact of the same type on the same table with equal columns in order. A foreign key also needs the same referenced table and the same referenced columns in order. A foreign-key fact with the same child columns but no referenced table or columns isunverifiable. A constraint type the introspector does not read isunverifiable.benchbox/platforms/duckdb_introspection.py). A second bounded query readsduckdb_constraints(), filtered in itsWHEREtoPRIMARY KEY/UNIQUE/FOREIGN KEY, the current database and schema, and the ledger's tables (bound parameters).NOT NULLandCHECKrows are never read. Columns are mapped by name from the cursor description, so a DuckDB build withoutreferenced_table/referenced_column_namesleaves foreign keysunverifiableinstead of failing; a build without the required columns, or a failed read, degrades the whole state; hitting the row bound marks it truncated.autoandtuned-fallback. These configurations no longer enable CHECK constraints on DuckDB (build_default_unified_configinbenchbox/cli/tuning_runtime.py), as the decision requires. The shipped DuckDB templates already disable CHECK. This changesrequested_config_hashand the cached database name for those DuckDB runs; the CHANGELOG notes it.Receipt entries for constraints carry
kind: "constraint"andconstraint_type. The referenced table and columns are only inevidenceand in thediff, which the exporter and Explorer sanitizers already drop, so no new identifying key reaches a redacted bundle.Type of Change
Testing
tests/unit/core/tuning/test_constraint_corroboration.py: classification of table-level, column-level, quoted and decoy forms; a positive case per constraint kind; negatives (primary key on different columns, foreign key with the same child columns and a different referenced table, different referenced columns, different child columns, column order, type confusion, other table, index fact);unverifiablecases (fact without referenced fields, no constraint source, unread type, constraint next to a corroborated sort key, degraded state); every fail-closed parse case.tests/unit/platforms/test_duckdb_introspection.py: real DuckDB catalog facts,NOT NULL/CHECKexclusion, ledger-table and current-schema bounding, foreign key to the wrong table, missing referenced columns, read failure, truncation, and the shipped TPC-H and SSB templates' schemas corroborating throughcreate_schema, plustest_quoted_unicode_table_name_constraints_are_readfor quoted non-ASCII tables.tests/integration/tuning/test_duckdb_constraint_corroboration_e2e.py(slow): a fullrun_benchmarkon TPC-H SF 0.01 with the shipped DuckDB template has 17 corroborated constraint entries, nounverifiableentry, and reachesapplied_verified.tests/unit/cli/test_tuning_runtime.py,tests/unit/cli/commands/test_run_tuning_mode_guard.py: CHECK is off for DuckDBauto/tuned-fallbackand unchanged elsewhere.uv run -- python -m pytest -m fast -q(36838 passed, 26 skipped, 788 warnings, 4 subtests passed),uv run -- ruff check .anduv run -- ruff format --check .passed, changed-filety checkpassed (repo-widety checkreports 664 diagnostics),make docs-validate,make comment-policy-check, andmake pr-preflight(214 passed, 3 deselected); the slow e2e with-m slow -n 0.benchbox run --platform duckdb --benchmark tpch --scale 0.01 --tuning tuned --phases generate,load,power --queries Q1reported 8unverifiableconstraint entries andapplied_unverifiedbefore, and 29 corroborated entries (8 primary keys, 9 foreign keys, 12 indexes) andapplied_verifiedafter.Public Contract Check
docs/reference/public-contracts.md, or this PR does not change public, beta-public, deprecated, generated, experimental, or repo-only contract surfaces. Receipt entries gain an optionalconstraint_typekey; no existing key changes.Documentation
docs/development/tuning-introspection-receipts.md)Code Quality
Artifact Hygiene
Soundness review:
This change touches soundness-manifest trust paths:
benchbox/core/tuning/introspection.pyandbenchbox/platforms/duckdb_introspection.py. It upgrades statements fromunverifiableto verdict-producing, so it can make a run more verified. External soundness review is requested through the requiredoracle-reviewcheck; do not auto-merge.Why a run cannot become more verified than its catalog shows:
unverifiable, which blocks." a "never corroborates a key ona. A table, column or referenced name that is not one bare or one double-quoted identifier blocks the statement.CREATE TABLEwhose text holds a literal the scanner cannot blank exactly (a dollar-quoted string$$...$$or$tag$...$tag$, an escape stringE'...', or a nested block comment) yields no constraint intent and staysunverifiableas a whole, so a parenthesis inside such a literal cannot end the column list early and hide aCHECK. The check is conservative: it can also reject a statement where these characters appear inside an ordinary string. No shipped benchmark schema contains them.CREATE TABLEholding one isunverifiableas a whole, otherwise a default such as'\')\''could expose a parenthesis and hide aCHECKwhile a surviving sort key verifies alone. Only the backslash-quote pair triggers, so a path such as'C:\data'still parses. No shipped benchmark schema contains the pair.[as list syntax and a backtick as an operator rather than identifier quotes, so aCREATE TABLEthat carries constraint text next to either character isunverifiableas a whole; otherwise a list default such as[']', ')', 'x']could end the scanner's bracket quote inside a string, expose a parenthesis and hide aCHECK. A statement without constraint text is unaffected, so layout-only intents keep working. No shipped benchmark schema combines them.SET schema,SET search_pathorUSE. Only tables with constraint intents are checked, so unrelated same-named tables do not block.duckdb_constraints()read takes its table names from the constraint intents, which use the same physical-identifier parser as the matching. Lookup parameters are lowered by DuckDB itself to match thelower(table_name)filter, so quoted tables such as"my t"and"q""t", and non-ASCII names such as"É", are found without broadening the shared ASCII-only identifier fold.CREATE TABLEwith a quoted table name gains constraint intents only; its layout clauses stayunverifiableas before.One deliberate tightening: on a platform without a constraint source, a
CREATE TABLEthat carries both a constraint and a corroborated sort, partition or cluster key used to verify on the layout clause alone while the constraint was ignored. It now records anunverifiableconstraint entry. No shipped template produces that shape (the ClickHouse templates disable primary keys, and Snowflake applies clustering throughALTER TABLE), so shipped tuned runs keep their status.Reviewers may want to check the column-list parser (
_constraint_intents,_column_level_constraints,_table_level_constraint, and the position alignment between the fully stripped text and the text that keeps double-quoted identifiers),_match_constraint, and theduckdb_constraints()query filters.Notes
git merge-treeshows no conflict). That PR's end-to-end testtest_duckdb_inline_keys_are_satisfied_and_undeclared_unique_constraints_not_droppedasserts that a DuckDB TPC-H run with only primary and foreign keys enabled isapplied_unverified. With constraint corroboration that run isapplied_verified, because its satisfied entries are non-blocking and itsCREATE TABLEstatements now corroborate; on a local merge of both branches that one assertion fails with'applied_verified' == 'applied_unverified'and every other test in both end-to-end files passes. Whichever PR merges second must change that assertion toAPPLIED_VERIFIED.CHECKorUNIQUE, and none renderson_delete_action, so a DuckDB TPC-H run whose tuning file enablescheck_constraintsorunique_constraintsnow reachesapplied_verifiedon its primary and foreign keys alone. Before this change those runs wereapplied_unverifiedonly because the key statements had no rule. Accounting for requested-but-unrendered toggles belongs to the reconciliation PR above, which treats a constraint type the benchmark does not declare as no request.constraint_type; DuckDBALTER TABLE ... ADD PRIMARY KEYstaysunverifiable.