fix(constraints): emit column and model constraints instead of dropping them - #828
Open
lll86789 wants to merge 3 commits into
Open
fix(constraints): emit column and model constraints instead of dropping them#828lll86789 wants to merge 3 commits into
lll86789 wants to merge 3 commits into
Conversation
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. |
…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
force-pushed
the
fix/579-constraints
branch
from
August 30, 2026 11:51
74df58e to
e0f1730
Compare
Author
|
Done — the DML refresh fix is now #829, sitting on What's left here is just the constraints work:
|
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
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.
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.