feat(sqlserver): Part 8 — dbt-sqlserver Jinja macro package - #18
Merged
Conversation
Vendors v1's macro tree (34 files) into crates/dbt-loader/src/dbt_macro_assets/dbt-sqlserver, mirroring dbt-fabric's directory layout rather than v1's, per the porting guide's delegate-to-fabric-where-behavior-matches instruction. Non-mechanical decisions, each checked against the v1 source, the shared dbt-adapters package, or the Rust adapter_impl.rs added in Part 7: - No sqlserver__generate_schema_name override: v2 has no per-adapter behavior-flag mechanism (Part 7 registered none for SqlServer), so the macro falls through to the shared default__generate_schema_name (standard target.schema_custom concatenation) rather than hardcoding v1's current no-prefix default. This is what dbt-msft/dbt-sqlserver#800 (filed last session) already assumes. - adapter_specific_behavior_flags() being empty also means adapter.behavior.dbt_sqlserver_use_dbt_transactions doesn't exist in v2. hooks.sql's two branches on that flag emit identical SQL either way, so the conditional was dropped rather than left referencing an undefined attribute. - full_refresh_build=prebuilt and table_refresh_method=dml (table.sql, incremental.sql) raise a compiler error instead of running: both depend on macros this PR doesn't port (create_table_as_prebuilt, table_dml_refresh), matching the loud-failure precedent set for custom indexes in Part 7's plan. - sqlserver__get_create_index_sql raises a compiler error naming the config option, rather than inheriting the shared default's silent no-op — a model with indexes: configured would otherwise build successfully with none. - Dropped everything gated on adapter.resolve_masks/apply_masks (table.sql, incremental.sql, snapshot.sql) and reconcile_indexes on persisted tables: neither has a v2 Rust counterpart, and since indexes: is already rejected loudly on first create, an already-existing table can't have a nonempty config to reconcile. - Skipped unit_test_create_table_as.sql and its check_for_nested_cte helper: v2's tests/unit.sql materialization dispatches through the same get_create_table_as_sql used by every other build path, not a separate unit_test_create_table_as macro — the v1 file is unreachable in v2's call graph. - Skipped materializations/models/view/create_view_as.sql (sqlserver__create_view_exec): grepped v1's own tree and found no caller; dead code inherited from an earlier Fabric-derived layout. - Ported adapters/catalog.sql, matching dbt-fabric/macros/adapters/catalog.sql still being live in v2 for `dbt docs generate` — the Part 5 Rust metadata module doesn't replace it. v1's version is kept over Fabric's: it fills table_comment/column_comment from sys.extended_properties, consistent with porting the full persist_docs.sql (which Fabric punts on entirely). - build_model_constraints(target_relation) added to table.sql and the full-refresh branch of incremental.sql (v1 doesn't call it, Fabric's newer materializations do) — v1's Python framework invoked model-constraint DDL outside the adapter macro; v2 needs the explicit call site. Verified: cargo check -p dbt-loader clean; cargo test -p dbt-loader --test main: 44 passed, 0 failed (pre-existing suite, unaffected by this change — no test coverage added here, matching Parts 1-7's precedent of leaving test authorship to a dedicated pass). Closes #8
…failure
Follows the view.rs/incremental.rs pattern (render the materialization
through MacroTestHarness, mock adapter calls, assert on what ran) rather
than a bare parse-only check — matching how postgres/databricks/spark are
covered, and consistent with this suite's existing adapters (only 5 of ~17
have view.rs coverage, 2 have incremental.rs; sqlserver had none).
Targets the two behaviors this PR actually changed relative to v1: the
rename-into-target / existing-renamed-to-backup swap that survives after
trimming the full_refresh_build=prebuilt and table_refresh_method=dml
branches, and sqlserver__get_create_index_sql raising instead of the shared
default's silent no-op when `indexes:` is configured.
Found along the way: MacroTestHarness's default_mock_config()'s generic
`config.get(key, default=...)` fallback only unwraps a *positional* second
arg; dbt-adapters' own create_indexes calls it with a keyword `default=[]`,
which the fallback doesn't see, so config.get('indexes', default=[]) doesn't
return the intended empty list. Not something to fix in shared harness code
for this PR — config_mock() here hardcodes indexes/contract explicitly and
only falls through to args[1] for genuinely positional call sites.
axellpadilla
changed the base branch from
part-7-sqlserver-adapter-impl
to
sqlserver-v2-port
August 3, 2026 01:18
4 tasks
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
1 participant
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.
Stacked on #17 (Part 7).
Vendors v1's 34-file macro tree into
crates/dbt-loader/src/dbt_macro_assets/dbt-sqlserver, mirroring dbt-fabric's directory layout rather than v1's. Non-mechanical decisions, each checked against v1 source, the shareddbt-adapterspackage, oradapter_impl.rs:sqlserver__generate_schema_nameoverride — falls through to the shareddefault__generate_schema_name(standard concatenation), since Part 7 registered no SqlServer behavior flags and v2 has no per-adapter flag mechanism to gate it. This is what Flipdbt_sqlserver_use_default_schema_concat's default toTruein the final 1.12 release dbt-msft/dbt-sqlserver#800 already assumes.hooks.sql's two branches onadapter.behavior.dbt_sqlserver_use_dbt_transactionsemit identical SQL either way, so the conditional (which would reference a nonexistent behavior flag in v2) was dropped rather than kept.full_refresh_build=prebuilt,table_refresh_method=dml, and a configuredindexes:all raise a compiler error naming the gap, rather than silently running a different path or no-op'ing — matching the loud-failure precedent from Part 7's plan for deferred features.adapter.resolve_masks/apply_masksand index reconciliation on persisted tables — neither has a v2 Rust counterpart, andindexes:is already rejected loudly on first create so a persisted table can't have a nonempty config left to reconcile.unit_test_create_table_as.sql(v2'stests/unit.sqldispatches through the sameget_create_table_as_sqlas every other build path — the v1 macro name is unreachable) andmaterializations/models/view/create_view_as.sql'ssqlserver__create_view_exec(no caller anywhere in v1's own tree — dead code).adapters/catalog.sql: dbt-fabric's equivalent is still live in v2 fordbt docs generate, and v1's version is kept over Fabric's since it fillstable_comment/column_comment, consistent with porting the fullpersist_docs.sql(which Fabric punts on entirely).build_model_constraints(target_relation)totable.sqland incremental's full-refresh branch — v1 doesn't call it (older framework), Fabric's newer materializations do; v2 needs the explicit call site for FK/PK contract enforcement to work.Every rewritten (not verbatim-copied) file was diffed against its v1 source to confirm only the intended removal/addition shows up — no accidental changes to retained logic.
Verified
cargo check -p dbt-loader: cleancargo test -p dbt-loader --test main: 47 passed (43 pre-existing + 4 new), 0 failedtests/materializations/table.rs) follow the existingview.rs/incremental.rspattern — render the materialization throughMacroTestHarness, mock adapter calls, assert on what ran — covering the two things this PR actually changed:table.sql's rename-swap surviving theprebuilt/dmlbranch removal, and theindexes:config now raising instead of silently no-op'ing.Closes #8