fix(constraints): emit column and model constraints instead of dropping them - #828
Conversation
|
Can we split the actual bug fix out into its own pull request, that needs to land even constraints is held up in review. |
…ng them Only `not_null` ever reached the database. `render_column_constraint` returned an empty string for every other type, and the macro its docstring pointed at as the place those were applied instead, `sqlserver__build_model_constraints`, was defined but called from nowhere, so model-level constraints were discarded outright. Where a constraint lands depends on whether it is named. An unnamed one renders inline in the CREATE TABLE column list, validated as the table is built, so a violation fails before the swap and leaves the previous table untouched. A named model-level constraint is applied by ALTER TABLE ... ADD CONSTRAINT after the build swaps the new table in and drops the old one, which is the first point at which the name is free: SQL Server scopes constraint names per schema (unlike index names, which are per table), so a name emitted inline would collide with the table being replaced on every rebuild after the first. A name on a column-level constraint is ignored with a warning pointing at the model-level form, and a foreign key that names no target warns rather than vanishing. Each ADD is guarded on the name already being present on the table, so the macro runs on every build path: a constraint added to a model that already exists lands on its next run instead of waiting for --full-refresh. A redefinition under an unchanged name is not detected - a constraint name, unlike a dbt_idx_ index name, is not a hash of its definition - and needs --full-refresh. Both are documented, as is the asymmetry that an unnamed constraint added to a model whose table persists is a silent no-op until then. Column-level CHECK constraints are hoisted into table-level clauses of the same CREATE TABLE: SQL Server accepts only one column-level CHECK per column. PRIMARY KEY and UNIQUE default to NONCLUSTERED so they coexist with the clustered columnstore index built for as_columnstore; dbt's own `expression` field overrides that, and anything other than those two keywords is rejected with a compile error rather than emitted as DDL that cannot parse. Foreign keys match the `to` / `to_columns` form as well as the older free-text `expression`, which was the only one recognised before. Unit-test fixture tables keep rendering `not_null` only, so a UNIQUE or FOREIGN KEY off the real contract cannot fail a unit test on stand-in data. Fixes dbt-msft#579 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aints table_refresh_method: dml falls back to a rename-swap whenever the model's schema changes, and that swap used to land a table built by SELECT * INTO, which carries no constraint and no NOT NULL. The rebuild that fixes it is a separate change; this asserts what it means for a contract-enforced model - that the named PRIMARY KEY, the inline CHECK and the NOT NULLs are all still on the table after a column is added. Also extends the columnstore/NOT NULL note in the as_columnstore section, and the corresponding changelog entry, to name the inline constraints that only carry across the swap once this change is in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
74df58e to
e0f1730
Compare
|
Done — the DML refresh fix is now #829, sitting on What's left here is just the constraints work:
|
|
@lll86789 please check conflict |
A named PRIMARY KEY / UNIQUE is applied by ALTER after the build has committed and swapped the table in, so an invalid `expression` on one only surfaced then, with the new table already in place. Validate it when the CREATE TABLE is rendered instead, the same moment an unnamed one fails. Docs: the nonclustered-index cost of a key applies to every contract-enforced model (they all load as CREATE TABLE + INSERT WITH (TABLOCK)), not only to full_refresh_build=prebuilt; the changelog pointed at table_refresh_method=dml as the foreign-key workaround, which the README correctly says is not one; and the README now explains the state a Msg 3726 failure leaves behind and how to get out of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/constraints.md states every rule the constraint handling is held to, with an identifier per rule and a table mapping each to the test that verifies it. test_constraints_e2e.py walks seven scenarios across runs - contract on/off, both violation modes, every "changing a constraint" bullet on a persisting table, the foreign-key parent rebuild and its recovery, the clustering override, the prebuilt path, several checks on one column, and dbt unit tests over a keyed contract - asserting the catalog after each step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed and validated locally against SQL Server 2025 (
One observation, not changed here: Backport to |
backport(1.11): emit column and model constraints instead of dropping them (#828)
Conflicts, and the semantic collision behind them, are all with dbt-msft#828 (constraints in contract-enforced builds), which landed on master after this branch was cut. - CHANGELOG.md: both sides rewrote the `table_refresh_method: dml` columnstore entry. Kept this branch's mechanics, which are the ones that will be true after the merge (the scratch table is an empty create plus a separate `INSERT ... WITH (TABLOCK)`, not a fused `SELECT * INTO`), and took master's wider scope, since after dbt-msft#828 that path loses declared constraints as well as NOT NULLs. Every other entry from both sides is kept as-is. - tests/functional/adapter/dbt/test_constraints.py: neither side's expected_sql was right after the merge. This branch's split the CREATE and the INSERT into separate EXEC batches but predates constraints being emitted at all; master's carries the constraints but still has both statements fused in one EXEC. Regenerated both fixtures from what the merged macros actually emit: a CREATE TABLE carrying the check constraints, then a separate EXEC for the TABLOCK insert - i.e. both behaviours, which is the point of the merge. The table-build macros themselves merged cleanly and correctly: columns_spec_ddl.sql is identical to master's, and the split create path in create.sql calls build_columns_constraints, so constraints survive the split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves #579
Constraints declared in a contract-enforced model's yaml never reached the database - only not_null was ever emitted. There are two independent breaks. render_column_constraint returned an empty string for every type but not_null, and sqlserver__build_model_constraints, which that method's docstring names as the place the other types are applied instead, has no call site anywhere in the repo, so model-level constraints were dropped outright. Each half reads as though the other one handles it.
Column-level constraints now render inline in the CREATE TABLE column list, and model-level constraints render there too when they carry no name:. A model-level constraint with a name: is applied by ALTER TABLE ADD CONSTRAINT once the build has swapped the new table into place and dropped the old one, which is the first point at which that name is free to reuse. SQL Server scopes constraint names per schema - unlike index names, which are scoped per table, which is what lets the existing deterministic dbt_idx_ naming work - so a name emitted inline collides with the table being replaced (Msg 2714) on every rebuild after the first. Keeping unnamed constraints inline is also worth something on its own: they are validated as the new table is built, so a violation fails the run before the swap and leaves the old table intact. A name: on a column-level constraint is ignored with a warning pointing at the model-level form.
PRIMARY KEY and UNIQUE render as NONCLUSTERED. SQL Server defaults an unqualified PRIMARY KEY to CLUSTERED, which cannot coexist with the clustered columnstore index built for as_columnstore (the default), so nonclustered is the safe default here. dbt's own expression field is the override - a constraint declaring expression: clustered keeps what it asked for - so no adapter-specific yaml key is needed. Anything else in that position is rejected at compile time rather than emitted as DDL that cannot parse.
Foreign keys now accept the to: / to_columns: form as well as the older free-text expression: form. Only the latter was matched before, so even a wired-up model constraint using to: - the form dbt-core actually produces from to: ref(...) - would have been silently discarded. A foreign key that names no target at all now warns instead of vanishing. Separately, column-level CHECK constraints are hoisted out of the column definition into table-level clauses in the same CREATE TABLE: SQL Server accepts only one column-level CHECK per column ("More than one column CHECK constraint specified for column ..."), while the table-level form has no such limit. Both are anonymous and both live in the same statement.
Each ALTER TABLE ADD CONSTRAINT is guarded on the name already being present on that table - sys.objects, matched on parent_object_id as well as name, so a same-named constraint on a sibling table cannot mask it - and the whole set is emitted as one batch. That makes build_model_constraints safe to call on every build path, including the ones that keep the existing table (a plain incremental run, a DML refresh), so a constraint added to an existing model lands on its next run instead of doing nothing until --full-refresh. What the guard cannot see is a constraint whose definition changes under an unchanged name: a constraint name, unlike a dbt_idx_ index name, is not a hash of its definition, so redefining one still needs --full-refresh. That, and the Msg 3726 a foreign key produces while the referenced model's backup table is dropped, are documented in the README.
The second commit fixes an independent bug found while testing the DML path. It affects models with no constraints at all: a model with table_refresh_method: dml lost its clustered columnstore index the first time its schema changed, and never got it back. That path builds a scratch table with SELECT * INTO and, when the columns no longer match, renames it into position - but SELECT * INTO copies no index and no constraint and takes nullability from the query, and create_indexes only builds what the indexes config names, never the as_columnstore CCI. So the model came back as a heap and stayed one, because every later run matched the new schema and took the DELETE+INSERT path; under an enforced contract the same rename also dropped its NOT NULLs and inline constraints. This looks like it could have been a deliberate trade, but the branch already calls create_indexes, and #641's tests for CCI preservation and for contract enforcement both change only data, never columns, so both stay on the DELETE+INSERT branch, while the one test that does change schema hardcodes as_columnstore: false. The three features were each covered and never crossed.
That branch now rebuilds the scratch through create_table_as before renaming it, which is how every other build path in the adapter creates a table. The rebuild is confined to that branch on purpose: doing it up front would build, and then throw away, a columnstore index on every steady-state refresh, which on a large table dominates the run, and a schema change is rare. It costs a second execution of the model's SQL on that run, the SELECT * INTO probe having already run it once. Probing the tmp view instead of the materialized scratch would avoid that, but it changes how the probe behaves and belongs in its own change.
Adds a functional suite that reads back sys.objects, sys.indexes and sys.masked_columns rather than asserting on generated SQL: a named constraint lands under its declared name and survives a full-refresh rebuild, a primary key on a columnstore table is nonclustered, expression: clustered is honoured, a constraint added to an existing model applies on the next run, an incremental model survives repeated runs, a column-level name: is ignored, a foreign key declared with to: ref(...) resolves, a named constraint coexists with a masked column, and a dml model keeps its columnstore index across a schema change with no contract involved. Adds unit tests for the renderers. Two expectations in the inherited dbt constraint tests encoded the dropped-constraint behaviour as expected SQL and are updated.
Verified locally against SQL Server 2022 (CU26) and SQL Server 2025 (RTM-CU8, 17.0.4075.5), with identical results on both: the unit tests and the full functional suite pass. The behaviour this depends on is the same across the two - a nonclustered primary key coexisting with the clustered columnstore index, constraint names being scoped per schema, and a named constraint applying to a column that carries a data mask.