feat(sqlserver): add dbt Fusion (v2) adapter support - #15769
Draft
axellpadilla wants to merge 16 commits into
Draft
feat(sqlserver): add dbt Fusion (v2) adapter support#15769axellpadilla wants to merge 16 commits into
axellpadilla wants to merge 16 commits into
Conversation
…atch
execute_inner splits a compiled statement() block into physical
statements and executes them in sequence, but kept only the literal
last one's result. A CREATE VIEW-based test macro shape
(EXEC('create view ...') -> select count(*) ... -> EXEC('drop
view ...')) needs multiple physical statements because CREATE VIEW
must be the sole statement in its batch on some engines, so the
trailing no-op DROP VIEW was clobbering the actual result with an
empty (0 rows, 0 columns) one.
Extracts the "which batch to keep" logic into LastBatchTracker: tracks
the literal last batch (for AdapterResponse's rows_affected/query_id,
which must still reflect what actually ran last) separately from the
last batch that has columns (for the returned AgateTable, only
relevant when fetch is requested), and adds direct unit tests for the
tracker covering the trailing-cleanup-statement case, the
nothing-has-columns fallback, and the fetch=false no-op case.
Per CONTRIBUTING.md's changelog requirement, flagged by the repo's bot on this PR.
Adds the `SqlServer` variant to `AdapterType` and the arms in `dbt-adapter-core` and `dbt-adapter-sql` that a new variant makes non-exhaustive. Nothing downstream is reachable without it, and it is what turns every unhandled `match adapter_type` in `dbt-adapter` into a compile error, which is how the remaining parts get their scope. `quote_char` is `"`, matching Fabric and dbt-sqlserver v1 — `SQLServerRelation` doesn't override `quote_character`, so v1 already renders identifiers with dbt-core's default. Double-quoted identifiers are only delimiters while `QUOTED_IDENTIFIER` is ON; setting it on connection open is part of the dbt-auth work, not this change. `max_identifier_length` is 128, SQL Server's documented limit for a regular identifier (`sysname` is `nvarchar(128)`). v1 enforces 127, but that constant came in with a copy of dbt-redshift's check and still carries its "Check for length of Redshift table/view names" comment, so this follows the database rather than v1. A 128-character relation name errors in v1 and builds here. `canonical_quote` already had a wildcard arm, so `SqlServer` was resolving to `QuotingStyle::Double` without being named. It is spelled out alongside `Fabric` so the choice is visible rather than incidental. strum's `serialize_all = "lowercase"` renders the variant as `sqlserver`, which is the name `internal_package_names()` will resolve the macro package from. Closes #1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the `// SqlServer,` placeholder in `DbConfig` with `SqlServer(Box<SqlServerDbConfig>)` and fills the nine `impl DbConfig` accessors plus `render_with_run_filter` that the new variant makes non-exhaustive. Field names and aliases follow v1's `SQLServerCredentials._ALIASES`, so an existing dbt-sqlserver profile parses unchanged. Omitted from it: `windows_login`/`trusted_connection`, since Windows auth is out of scope for the initial port; `backend`, since v1's choice of pyodbc/mssql-python/ADBC has no v2 equivalent; and `xact_abort`, which is a session setting rather than a connection parameter. `get_connection_keys` lists `host` where v1 and `FabricDbConfig` list `server`. `to_connection_mapping` filters serialized key names and `#[serde(alias)]` only affects deserialization, so `server` matches nothing — Fabric's host silently never reaches `dbt debug`. Same reasoning keeps `PWD` and `client_secret` off the list; a test pins both halves. `threads` is on the struct and wired through `get_threads`/`set_threads`, unlike Fabric which has no such field. v1 inherits it from `Credentials`, so a ported profile setting `threads:` would otherwise be silently ignored. `SqlServerTargetEnv` exposes host, port and user alongside Fabric's `authentication`. `authentication` is left empty rather than defaulted when unset: v1 defaults to `sql`, but what actually governs connection setup is `dbt-auth` `parse_auth`, whose `DEFAULT_AUTH` is Entra service-principal today. That default is the auth part's call, not this one. Closes #2 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SQLServerAuth` reached the driver with Entra flows only, and built a URI carrying just host, port and database. A native SQL Server login -- the default in every dbt-sqlserver v1 profile -- had no path through it, and `encrypt` / `trust_cert` / `login_timeout` were parsed into the profile struct and then dropped. `SqlLogin` is a new top-level `SQLServerAuthIR` variant rather than a refinement of an existing one: a server-local login is a different authentication contract from a federated Entra token, not a subtype of it. It sets `user id` and `password` with no `fedauth`, reading `UID` and `PWD` the same way `ActiveDirectoryPassword` does. `authentication: sql` selects it, case-insensitively, alongside the existing `serviceprincipal` normalization. `apply_connection_args` now emits `encrypt`, `TrustServerCertificate` and `connection timeout`. Values and defaults come from v1's `build_adbc_connection_uri`, which builds the same query string against the same driver: `encrypt=true`, `trust_cert=false`, and `login_timeout` omitted when it is not positive. Both flags accept a YAML boolean or its string spelling, and an unrecognized value falls back to the default rather than erroring -- in both cases the default is the safe direction. Measured against a live SQL Server 2022 through the same go-mssqldb ADBC driver v1.6.0 that dbt installs, using the URI shape this produces: - the SQL login connects, and a `!` in the password survives the query-pair encoding - `connection timeout=30` is accepted - `encrypt=true` with `TrustServerCertificate=false` fails the handshake against a self-signed certificate, which is what these parameters exist to let an on-prem profile opt out of Ten unit tests cover the new paths. `DEFAULT_AUTH` is unchanged: it is shared with Fabric, which maps to the same backend. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two exhaustive matches stopped compiling when Part 1 added the variant -- `create_static_relation` and `backend_of` -- and `relation_impl.rs` has none, so every arm it needs is a silent-correctness one that shows up as wrong SQL rather than a build failure. `backend_of` maps `SqlServer` to `Backend::SQLServer`, the same ADBC backend Fabric rides. No adapter can be constructed without it, and no issue in the series claimed it. In `relation_impl.rs`: - `get_database` joins the group that raises `InvalidConfig` when `database` is unset. The `_` arm returns an empty string, so without this a relation missing a database renders as `.schema.table`. - `get_canonical_fqn` joins `Fabric | Bigquery`, which pass an unquoted path part through verbatim. SQL Server stores the case it was given. `normalize_component` is deliberately left alone: it models how the server resolves an unquoted name, and folding to lower case is the right model for a case-insensitive collation, which is where Fabric already sits. - `quoted` doubles an embedded delimiter for `SqlServer`. The shared default interpolates verbatim, so `x"q` rendered `"x"q"` -- unparseable, and v2 would have rejected names v1 accepts. The override is scoped to this adapter rather than fixed in `dbt-schemas`, where it would change rendering for every adapter that quotes. - `new_sqlserver` mirrors `new_fabric`; its callers arrive with the metadata module. `include_policy` needs nothing: `_ => Policy::trues()` is already right for 3-part naming, and the explicit arms there are all adapters that drop a path part. Five tests in a `sqlserver` module: three-part rendering, the missing database error, delimiter doubling, case preservation through `get_canonical_fqn`, and construction through `RelationStatic`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Modeled on `metadata/fabric/mod.rs`, but not a copy of it. Fabric drives `sp_tables` / `sp_columns`, and two things measured against SQL Server 2022 rule that out here: - The procedures read the connection's current database only. Any other `@table_qualifier` is error 15250, "The database name component of the object qualifier must be the name of the current database". A dbt project that reaches across databases on one server is ordinary on SQL Server and impossible on Fabric, where a warehouse is its own database. - `@table_name` is a `LIKE` pattern unless `@fUsePattern = 0` is passed, so `stg_orders` also matches `stgXorders`. Underscores are everywhere in dbt model names, and the second row makes `get_relation` fail the one-row check rather than return the wrong type. `\_` does not escape it; only `@fUsePattern = 0` does. The catalog views take a three-part name, which resolves cross-database and, unlike the `USE` that v1 emits before every metadata query, leaves the connection's current database alone. That matters for a pooled connection. `sys.columns` also carries the type detail `sp_columns` drops: `TYPE_NAME` is the bare name, so `decimal(18,4)` arrives as `decimal` and `nvarchar(50)` as `nvarchar`. `compose_type_text` rebuilds the declared text from `max_length` / `precision` / `scale`, following v1's `get_columns_in_relation` -- including its halving of `max_length` for the national types, which `sys.columns` reports in bytes. `datetime` reports a scale but rejects one. `build_schemas_from_stats_sql` and `build_columns_from_get_columns` read `table_comment` and `column_comment`, as Postgres does and Fabric does not; v1's `get_catalog` returns both, and v1 supports `persist_docs`. `freshness_inner` stays `todo!()`, matching Fabric and Postgres, though v1 has `get_relation_last_modified` to port. `metadata_adapter` and `list_relations` in `adapter_impl.rs` are two of Part 7's arms, taken here because they are what makes this module reachable. Ten tests over the SQL builders and the type composer. Every query was also run against a live SQL Server 2022 from a connection attached to a different database, in the exact text the Rust emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds SqlServer arms to sql_types.rs (format_arrow_type_as_sql dispatch, the convert_*_type consolidation match, field_comment, build_sdf_schema, max_varchar_size/max_varbinary_size) and column_builder.rs (build, build_from_parts), mirroring the Fabric arms where SQL Server's T-SQL surface agrees and diverging where it measurably doesn't. Values are checked against dbt/adapters/sqlserver/sqlserver_adapter.py and sqlserver_column.py (v1's actual behavior, not its deprecated legacy path — dbt_sqlserver_use_native_string_types defaults True as of 1.12): convert_number_type's "float" if decimals else "int" rule, convert_boolean_type's "bit", convert_datetime_type's "datetime2(6)", convert_time_type's "time(6)". STRING maps to VARCHAR(MAX), not Fabric's VARCHAR(8000): Fabric Warehouse tracks its own byte cap here, and on-prem/Azure SQL Database has no equivalent one — VARCHAR(MAX) is v1's native mapping. A new sqlserver module (mirroring the fabric one) backs the format_arrow_type_as_sql dispatch and build_sqlserver's type_ops.format_arrow_type_as_sql call. get_field_sql_type_metadata_key gets `todo!()`, matching its other untouched arms (Databricks, Postgres, Salesforce, Spark, DuckDB, Alt): the function is dead outside its own Bigquery-only test, and the answer that matters lives in dbt_adapter_sql::types::SQLSERVER_KEYS. Leaves Part 7's scope (adapter_impl.rs's SqlServer arms) untouched; those E0004s are unrelated to this file pair and still block a full crate build. Closes #6
Resolves all 33 E0004 non-exhaustive-match errors in adapter_impl.rs
that blocked a full dbt-adapter crate build, by adding a SqlServer arm
(or joining an existing group) at every site the compiler flagged.
Grouping decisions, verified against v1 dbt-sqlserver and the shared
macro packages rather than assumed identical to Fabric:
- valid_incremental_strategies: matches Fabric's set exactly — v1's
SQLServerAdapter.valid_incremental_strategies() returns the same
four strategies.
- list_schemas_inner's schema column name ("schema"): confirmed via
v1's sqlserver__list_schemas, which selects `name as [schema]`.
- standardize_grants_dict: joins the Postgres/Bigquery/DuckDB/Alt
group (grantee/privilege_type columns), not the "grants not
implemented" bucket — v1's sqlserver__get_show_grant_sql selects
exactly those two column names, and SQLServerAdapter doesn't
override standardize_grants_dict, so it falls through to that same
base implementation.
- truncate_relation and get_columns_in_relation's macro-dispatch
group: v1 has sqlserver__truncate_relation and
sqlserver__get_columns_in_relation macros, so SqlServer joins the
macro-based groups rather than any unimplemented bucket.
- get_constraint_support: v1's SQLServerAdapter.CONSTRAINT_SUPPORT
overrides all five constraint types to Enforced (including Check,
unlike Fabric which marks Check NotSupported) — ported as its own
block rather than folded into Fabric's.
- verify_database: this method is only invoked from the dbt-postgres
and dbt-redshift macro packages (grep confirms no other package
calls adapter.verify_database), which SQL Server's macro package
never dispatches into, so SqlServer joins the "unimplemented"
bucket alongside Fabric/Snowflake/Databricks rather than the
Postgres/DuckDB/Alt/ClickHouse single-database-check group — despite
SQL Server otherwise supporting cross-database three-part names.
- Every purely BigQuery- or Databricks-specific method (partition
config, dataset location, table options, DBR capabilities, Iceberg
helpers, etc.) gets SqlServer added to the existing "unimplemented"
bucket alongside every other non-BigQuery/non-Databricks adapter.
- adapter_specific_behavior_flags: returns vec![] for SqlServer. v1
registers 5 behavior flags, but 4 (default_schema_concat,
native_string_types, safe_type_expansion, use_dbt_transactions) are
staying on their v1 default with no alternate code path in the Rust
adapter yet, and the 5th (disable_empty_relation_aliases) is out of
scope here — so none are registered rather than declaring toggles
the adapter doesn't yet honor. Follow-up if/when those paths land.
Closes #7
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.
Adds SqlServerDbConfig's InteractiveSetup impl, modeled on fabric_config.rs's auth-branching pattern but covering all four authentication methods dbt-auth/src/sqlserver/mod.rs actually implements (sql, ActiveDirectoryServicePrincipal, ActiveDirectoryPassword, environment) rather than Fabric's pared-down two. Wires SqlServer into get_available_adapters() and the create_profile_for_adapter() match arm. Closes #9
Ran a clean dbt build against a local SQL Server 2022 container and a
ported copy of jaffle-shop (seed, run, test, unit test). Along the way,
fixed the SQL Server-specific gaps that blocked it:
- dbt-df-providers/src/seed_io.rs: missing AdapterType::SqlServer arm
in infer_seed_column_name_strategy (compile error). Grouped with
Fabric's Verbatim strategy -- SQL Server's default collation is
case-insensitive, so seed column names shouldn't be case-folded.
- dbt-tasks-sa/src/sql/dialect.rs: missing AdapterType::SqlServer arm
in sqlparser_dialect_for (compile error). Points it at the same
MsSqlDialect Fabric already uses.
- dbt-adapter/src/formatter.rs: format_sql_with_bindings/format_bool
only special-cased Fabric for the `?` binding placeholder and 1/0
bool literals, even though sqlserver__get_binding_char() emits the
same `?` Fabric does -- broke every seed insert.
No changelog entry: CONTRIBUTING.md's changelog process targets PRs
against dbt-labs/dbt-core's main, and the closest precedent for a
mid-rollout adapter milestone ("Add ClickHouse to the list of non
experimental adapters") shows this isn't a shippable end-user feature
yet either. Better added once, for the whole adapter, on the eventual
upstream PR.
Auth-mode matrix beyond plain SQL isn't covered here -- no Azure AD
credentials available in this environment for the service principal /
AD password / environment credential flows -- and #adapter-ecosystem
CI coordination is left to the user, per the issue.
Two further defects were found during the smoke test and confirmed
against a live instance to be pre-existing gaps shared with Fabric
(not SQL-Server-specific, and not fixed here): dbt_utils's
expression_is_true selects an unaliased literal, which T-SQL rejects
from a named derived table/view regardless of adapter; and
dbt.date_spine/generate_series nests a WITH block inside another CTE's
body, which T-SQL also rejects. Drafted as
issues/v2-dbt-utils-expression-is-true-unnamed-column-tsql.md and
issues/v2-date-spine-nested-cte-tsql.md in the roadmap repo, for
follow-up against dbt-labs/dbt-utils and dbt-labs/dbt-core
respectively.
Two more general defects, unrelated to anything SQL-Server-specific,
were found and fixed on top -- see the next two commits.
Closes #10
…o truthiness get_test_results/get_column_test_result read should_warn/should_error via minijinja's generic Value::is_true(), which treats any non-empty string as true. SQL Server has no boolean literal, so sqlserver__get_test_sql emits the text 'true'/'false' for these columns -- meaning 'false' was read as true, and every test reported should_error regardless of actual failure count. Adds value_to_test_bool() to parse recognized true/false text explicitly first, matching what v1's convert_bool_type() already does via strtobool(). This is general engine code, not SQL Server-specific, so it's drafted as an upstream issue in the roadmap repo rather than folded into Part 10's SQL Server-scoped commit: issues/v2-test-result-bool-parsing-truthiness.md
…'s three-part name sqlserver_get_relation quoted the raw database argument straight into build_get_relation_sql with no check, unlike quoted_database() in metadata/sqlserver/mod.rs, which the same commit relies on elsewhere. An empty database silently built a malformed `"".sys.objects` query instead of failing with the clear AdapterError the sibling guard already gives.
Per CONTRIBUTING.md's changelog requirement, for the upstream PR against dbt-labs/dbt-core:main closing dbt-labs#15714.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Bumpy Road Ahead)
(9 files with Complex Method, Large Method, Large Assertion Blocks, Code Duplication, Lines of Code in a Single File, Excess Number of Function Arguments)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| sqlserver_config.rs | 1 critical rule | 9.46 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 2 advisory rules | 9.10 | Suppress |
| sql_types.rs | 2 advisory rules | 6.56 → 5.82 | Suppress |
| mod.rs | 1 advisory rule | 9.39 → 8.82 | Suppress |
| sqlserver_config.rs | 1 advisory rule | 9.46 | Suppress |
| profiles.rs | 3 advisory rules | 5.04 → 4.68 | Suppress |
| relation_impl.rs | 2 advisory rules | 5.88 → 5.55 | Suppress |
| column_builder.rs | 2 advisory rules | 7.70 → 7.67 | Suppress |
| profile_setup.rs | 1 advisory rule | 7.57 → 7.54 | Suppress |
| adapter_impl.rs | 2 advisory rules | 1.69 → 1.68 | Suppress |
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| } | ||
|
|
||
| let mut last_batch = None; | ||
| let mut tracker = LastBatchTracker::default(); |
There was a problem hiding this comment.
❌ Getting worse: Complex Method
AdapterImpl.execute_inner already has high cyclomatic complexity, and now it increases in Lines of Code from 85 to 94
Comment on lines
+4285
to
+4287
| Impl(SqlServer, engine) => { | ||
| sqlserver::list_relations(engine.as_ref(), query_ctx, conn, db_schema, token) | ||
| } |
There was a problem hiding this comment.
❌ Getting worse: Large Method
AdapterImpl.list_relations increases from 80 to 83 lines of code, threshold = 70
Comment on lines
+2215
to
+2238
| DbConfig::SqlServer(config) => { | ||
| let threads = match config.threads { | ||
| Some(StringOrInteger::String(threads)) => Some( | ||
| threads | ||
| .parse::<u16>() | ||
| .map_err(|_| "threads must be a positive integer".to_string())?, | ||
| ), | ||
| Some(StringOrInteger::Integer(threads)) => Some(threads as u16), | ||
| None => None, | ||
| }; | ||
|
|
||
| Ok(TargetContext::SqlServer(SqlServerTargetEnv { | ||
| host: config.host, | ||
| port: config.port, | ||
| user: config.user, | ||
| authentication: config.authentication.unwrap_or_default(), | ||
| __common__: CommonTargetContext { | ||
| database: config.database.ok_or_else(|| missing("database"))?, | ||
| schema: config.schema.ok_or_else(|| missing("schema"))?, | ||
| type_: adapter_type, | ||
| threads, | ||
| }, | ||
| })) | ||
| } |
There was a problem hiding this comment.
❌ Getting worse: Complex Method
TargetContext.try_from increases in cyclomatic complexity from 40 to 43, threshold = 9
Comment on lines
+278
to
+291
| DbConfig::SqlServer(_) => &[ | ||
| "host", | ||
| "port", | ||
| "database", | ||
| "schema", | ||
| "UID", | ||
| "authentication", | ||
| "retries", | ||
| "login_timeout", | ||
| "query_timeout", | ||
| "trace_flag", | ||
| "encrypt", | ||
| "trust_cert", | ||
| ], |
There was a problem hiding this comment.
❌ Getting worse: Large Method
DbConfig.get_connection_keys increases from 172 to 186 lines of code, threshold = 70
Comment on lines
+2645
to
+2670
| fn test_sqlserver_config_parses() { | ||
| let config: DbConfig = dbt_yaml::from_str( | ||
| "type: sqlserver\n\ | ||
| host: sql.prod.internal\n\ | ||
| port: 1433\n\ | ||
| database: analytics\n\ | ||
| schema: dbo\n\ | ||
| UID: alice\n\ | ||
| PWD: hunter2\n\ | ||
| authentication: sql\n\ | ||
| threads: 4\n", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let DbConfig::SqlServer(sqlserver_config) = config else { | ||
| panic!("Expected DbConfig::SqlServer"); | ||
| }; | ||
| assert_eq!(sqlserver_config.host, Some("sql.prod.internal".to_string())); | ||
| assert_eq!(sqlserver_config.port, Some(StringOrInteger::Integer(1433))); | ||
| assert_eq!(sqlserver_config.database, Some("analytics".to_string())); | ||
| assert_eq!(sqlserver_config.schema, Some("dbo".to_string())); | ||
| assert_eq!(sqlserver_config.user, Some("alice".to_string())); | ||
| assert_eq!(sqlserver_config.password, Some("hunter2".to_string())); | ||
| assert_eq!(sqlserver_config.authentication, Some("sql".to_string())); | ||
| assert_eq!(sqlserver_config.threads, Some(StringOrInteger::Integer(4))); | ||
| } |
There was a problem hiding this comment.
❌ Getting worse: Large Assertion Blocks
The number of large assertion blocks increases from 8 to 12, threshold = 4
Comment on lines
+1036
to
+1037
| | SqlServer | ClickHouse | Exasol | Starburst | Athena | Trino | Dremio | Oracle | ||
| | Datafusion => None, |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 8 functions with similar structure: fabric.try_format_type,max_varbinary_size,max_varchar_size,snowflake.is_time and 4 more functions
| Bigquery => "float64", | ||
| Databricks => "float", | ||
| Fabric => "real", | ||
| Fabric | SqlServer => "real", |
There was a problem hiding this comment.
❌ Getting worse: Complex Method
DefaultTypeOps.write_sql_type_for_dbt_convert_functions already has high cyclomatic complexity, and now it increases in Lines of Code from 93 to 95
Comment on lines
+83
to
+146
| fn set_field(&mut self, field_name: &str, value: FieldValue) -> FsResult<()> { | ||
| match field_name { | ||
| "host" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.host = Some(s); | ||
| } | ||
| } | ||
| "port" => match value { | ||
| FieldValue::String(s) => { | ||
| if let Ok(port) = s.parse::<i64>() { | ||
| self.port = Some(StringOrInteger::Integer(port)); | ||
| } | ||
| } | ||
| FieldValue::Integer(i) => { | ||
| self.port = Some(StringOrInteger::Integer(i)); | ||
| } | ||
| _ => {} | ||
| }, | ||
| "database" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.database = Some(s); | ||
| } | ||
| } | ||
| "schema" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.schema = Some(s); | ||
| } | ||
| } | ||
| "authentication" => { | ||
| if let FieldValue::Integer(i) = value | ||
| && let Some((val, _)) = AUTH_METHODS.get(i as usize) | ||
| { | ||
| self.authentication = Some((*val).to_string()); | ||
| } | ||
| } | ||
| "user" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.user = Some(s); | ||
| } | ||
| } | ||
| "password" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.password = Some(s); | ||
| } | ||
| } | ||
| "client_id" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.client_id = Some(s); | ||
| } | ||
| } | ||
| "client_secret" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.client_secret = Some(s); | ||
| } | ||
| } | ||
| "tenant_id" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.tenant_id = Some(s); | ||
| } | ||
| } | ||
| _ => {} // Ignore temporary or unrecognized fields | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
SqlServerDbConfig.set_field has a cyclomatic complexity of 14, threshold = 9
Comment on lines
+83
to
+146
| fn set_field(&mut self, field_name: &str, value: FieldValue) -> FsResult<()> { | ||
| match field_name { | ||
| "host" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.host = Some(s); | ||
| } | ||
| } | ||
| "port" => match value { | ||
| FieldValue::String(s) => { | ||
| if let Ok(port) = s.parse::<i64>() { | ||
| self.port = Some(StringOrInteger::Integer(port)); | ||
| } | ||
| } | ||
| FieldValue::Integer(i) => { | ||
| self.port = Some(StringOrInteger::Integer(i)); | ||
| } | ||
| _ => {} | ||
| }, | ||
| "database" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.database = Some(s); | ||
| } | ||
| } | ||
| "schema" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.schema = Some(s); | ||
| } | ||
| } | ||
| "authentication" => { | ||
| if let FieldValue::Integer(i) = value | ||
| && let Some((val, _)) = AUTH_METHODS.get(i as usize) | ||
| { | ||
| self.authentication = Some((*val).to_string()); | ||
| } | ||
| } | ||
| "user" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.user = Some(s); | ||
| } | ||
| } | ||
| "password" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.password = Some(s); | ||
| } | ||
| } | ||
| "client_id" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.client_id = Some(s); | ||
| } | ||
| } | ||
| "client_secret" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.client_secret = Some(s); | ||
| } | ||
| } | ||
| "tenant_id" => { | ||
| if let FieldValue::String(s) = value { | ||
| self.tenant_id = Some(s); | ||
| } | ||
| } | ||
| _ => {} // Ignore temporary or unrecognized fields | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
SqlServerDbConfig.set_field has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
Comment on lines
+397
to
+403
| AdapterType::SqlServer => { | ||
| let sqlserver_config = match existing_config { | ||
| Some(DbConfig::SqlServer(config)) => Some(config), | ||
| _ => None, | ||
| }; | ||
| DbConfig::SqlServer(setup_sqlserver_profile(sqlserver_config.map(Box::as_ref))?) | ||
| } |
There was a problem hiding this comment.
❌ Getting worse: Complex Method
ProfileSetup.create_profile_for_adapter increases in cyclomatic complexity from 11 to 12, threshold = 9
|
Putting draft into project backlog |
5 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
3 participants
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.
Closes #15714.
Summary
Adds
AdapterType::SqlServerto dbt Fusion: profile config, authentication(Entra + native SQL login), relation quoting, catalog introspection, SQL
type mapping, the
dbt-adaptermatch-arm surface, the vendored macropackage, and an optional
dbt initwizard. Ships gated behindDBT_ALLOW_EXPERIMENTAL_ADAPTERS=true(not added toNON_EXPERIMENTAL_ADAPTERS), same as every other adapter's initiallanding.
Landed as ten sequential, crate-scoped PRs against a staging branch first
(
sqlserver-v2-porton dbt-sqlserver-next/dbt-core,#11–#20) —
registering the adapter type makes every exhaustive
match adapter_type()across the workspace non-exhaustive, so it can't land upstream piecemeal
without breaking
main's build in between. Individual PR descriptionsbelow cite the specific v1/live-server evidence behind each call; this
summary groups by decision.
Decisions worth a reviewer's eye
Quoting:
quote_char = '"'withQUOTED_IDENTIFIERconfirmed ON bydefault on a live SQL Server 2022 (no init SQL needed) — matches
Fabricand matches what v1 already renders despite T-SQL also accepting
[brackets].quoted()doubles an embedded delimiter forSqlServerspecifically (the shared default renders
x"qas unparseable"x"q");scoped narrowly rather than fixed in the shared default, which changes
rendering for every quoting adapter.
Identifier length:
max_identifier_length = 128, not v1's 127 — SQLServer's documented and live-verified limit (
sysnameisnvarchar(128));v1's 127 traces to a copy-pasted Redshift constant, comment included. A
128-character name that v1 rejects builds here; tracked as a v1-side bug
separately, not changed to match v1.
Auth: native SQL login (
authentication: sql) added as a new top-levelSQLServerAuthIRvariant alongside the existing Entra flows (serviceprincipal, AD password, environment credential — already implemented and
tested pre-port).
encrypt/TrustServerCertificate/connection timeoutported from v1's own ADBC backend
(dbt-msft/dbt-sqlserver#783,
same driver), live-verified against a self-signed on-prem instance.
Windows/trusted-connection auth and named-instance hosts (
host\instance)are deferred — the latter fails loudly at URI parse rather than shipping an
unverified rewrite.
Catalog introspection: uses
sys.objects/sys.schemas/sys.columns/sys.typesdirectly rather than
Fabric'ssp_tables/sp_columnspattern — measuredagainst live SQL Server 2022 that those procedures are single-database
(cross-database reads, which SQL Server supports and v1 relies on, error
out) and that
@table_nameis aLIKEpattern, not an exact match(
probe_tablevsprobeXtable). Both are pre-existing gaps inFabric'sown module too; not touched here since only the pattern half is verifiable
against a T-SQL engine this checkout can reach, and cross-database is
verifiably impossible for Fabric's model to hit at all. Filed as a
follow-up, not fixed in this PR.
Type mapping:
STRING→VARCHAR(MAX)(v1's current native-stringdefault, not
Fabric's byte-cappedVARCHAR(8000)— SQL Server has noequivalent cap tracked here); decimal →
float/intkeyed by scale,reproducing v1's threshold rather than
Fabric's unconditionalfloat.Macro package: 34-file v1 tree vendored, mirroring
dbt-fabric'slayout.
indexes:config,full_refresh_build: prebuilt, andtable_refresh_method: dmlall raise a named compiler error rather thansilently no-op'ing or running a different path. Dynamic data masking and
index reconciliation on persisted tables dropped entirely (no v2 Rust
counterpart to call).
Not registered:
adapter_specific_behavior_flagsreturnsvec![]—v1's five behavior flags (native string types, safe type expansion, dbt
transactions, default schema concat, empty relation aliases) all stay on
their v1 default with no alternate Rust code path yet, so nothing is
declared that the adapter can't actually honor.
Deferred / explicitly out of scope
host\instance)full_refresh_build: prebuilt, scalar function materializations, table clone supportSET XACT_ABORT ONconnection-init SQL (currently off by default; severalvendored macros assume it's on — v1 issues it, v2 doesn't yet, and there's
no per-connection init hook to hang it on)
normalize_componentalways folds tolowercase; wrong under a case-sensitive collation — matches a known,
skipped-in-CI v1 defect, not a regression)
None of these were silently dropped — each is cited against the specific
v1 behavior or plan decision it diverges from in the individual Part PRs
(
dbt-sqlserver-next/dbt-core#11–#20) and in05-open-questions-and-risks.mdin the roadmap repo.
Verified
cargo build -p dbt-adapter-core -p dbt-adapter-sql: cleancargo nextest run -p dbt-schemas: 446 passed / 0 failedcargo nextest run -p dbt-auth: 287 passed / 0 failedcargo test -p dbt-adapter --lib: 891 passed / 0 failedcargo test -p dbt-loader --test main: 47 passed / 0 failed (43 pre-existing + 4 new)cargo test -p dbt-init: 7/7 passing (no regression; no new tests — noadapter_config/*.rsfile in the crate has coverage today,fabric_config.rsincluded)cargo fmt --check/cargo clippy --all-targets: clean throughoutcargo build -p dbt-sa-cli: cleandbt build(seed, run, generic tests, unit tests)against a local SQL Server 2022 container and a T-SQL-ported
jaffle-shop, plain SQL auth.
Auth-mode matrix beyond plain SQL (service principal, AD password,
environment credential) not exercised end-to-end — no Azure AD
credentials available in this environment; each is unit-tested in
dbt-authindividually.Bugs found outside SQL Server's own code, filed separately
Two pre-existing gaps shared with
Fabric(same T-SQL engine, sameunmodified shared macros), not fixed here:
dbt_utils.expression_is_trueselects an unaliased literal, which T-SQL rejects from any named derived
table or view;
dbt.date_spine/generate_seriesnests aWITHblockinside another CTE, which T-SQL also rejects, breaking
metricflow_time_spine. Roadmap-repo drafts:issues/v2-dbt-utils-expression-is-true-unnamed-column-tsql.md,issues/v2-date-spine-nested-cte-tsql.md.Note on two out-of-scope commits in this diff
This branch also carries
16152a55f/2343c50b4(a multi-statement querybatch fix) and
fe6b636df(a test-boolean-parsing fix) — general dbtFusion engine bugs found while smoke-testing this adapter, not part of the
SQL Server port itself. Each is filed and reviewable on its own:
#15765/#15766
for the first, #15767/#15768
for the second. Please review those separately rather than as part of this
adapter's diff; they'll drop out of this branch's history the next time it
syncs with
mainafter#15766and#15768merge.