Skip to content

fix(constraints): emit column and model constraints instead of dropping them - #828

Open
lll86789 wants to merge 3 commits into
dbt-msft:masterfrom
lll86789:fix/579-constraints
Open

fix(constraints): emit column and model constraints instead of dropping them#828
lll86789 wants to merge 3 commits into
dbt-msft:masterfrom
lll86789:fix/579-constraints

Conversation

@lll86789

Copy link
Copy Markdown

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.

@Benjamin-Knight

Copy link
Copy Markdown
Collaborator

Can we split the actual bug fix out into its own pull request, that needs to land even constraints is held up in review.

lll86789 and others added 3 commits August 30, 2026 19:34
…does

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 and, when the columns no longer match, renames
it into position. The scratch is built with SELECT * INTO, which copies no
index and no constraint and takes nullability from the query; create_indexes
then builds only 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 the model's NOT NULLs.

That branch now rebuilds the scratch through create_table_as before renaming
it, which is simply how this adapter creates a table, so the columnstore index
and the full column DDL carry across the swap. SELECT * INTO stays as the
schema probe, and the rebuild is confined to the branch that actually renames:
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. A schema
change is rare, so one extra build there is much the cheaper trade.

The cost on that run is a second execution of the model's SQL - the probe
having already run it once - plus the extra columnstore build. Probing the tmp
view rather than the materialized scratch would avoid the double execution, but
that changes how the probe behaves and belongs in its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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>
@lll86789

Copy link
Copy Markdown
Author

Done — the DML refresh fix is now #829, sitting on master, so it can go in whenever it's ready.

What's left here is just the constraints work:

  • fix(constraints): emit column and model constraints instead of dropping them — unchanged.
  • test(constraints): cover the dml refresh's rename-swap keeping constraintsTestDmlRefreshKeepsConstraints, checking that the named PRIMARY KEY, the inline CHECK and the NOT NULLs all survive the rename-swap. It only means anything once the constraint emission in this PR is in, so it stayed; the contract-free columnstore test went over to fix: rebuild the DML refresh's scratch table so a schema change keeps the columnstore #829.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Column constraints (Primary Key, Foreign Keys, Checks, Unique) and Model constraints are not generated

2 participants