fix: add explicit sqlserver__ dispatch to T-SQL fabric macros (CORE-877)#1013
Merged
Conversation
dbt-sqlserver removed dbt-fabric from its adapter dispatch chain, so sqlserver targets no longer inherit fabric__ macros. Delegate sqlserver__ to the existing T-SQL implementations to fix CI failures such as Invalid column name 'True' when building dbt_invocations on SQL Server. CORE-877 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds many ChangesSQL Server Adapter Dispatch for Fabric T-SQL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macros/utils/cross_db_utils/sqlserver_tsql_dispatch.sql`:
- Around line 81-87: The wrapper macro sqlserver__create_temp_table currently
calls the delegated macro elementary.fabric__create_temp_table with {% do ... %}
which discards its return value; change the call to return the delegated result
(e.g., replace {% do elementary.fabric__create_temp_table(...) %} with {{
return(elementary.fabric__create_temp_table(...)) }} or simply {{
elementary.fabric__create_temp_table(...) }} depending on your macro
conventions) so the relation/SQL is propagated to callers; apply the same change
to the other wrapper(s) that delegate to elementary.fabric__create_temp_table
(the similar block around the other occurrence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d33a9e59-dc7c-4261-9ebf-340db82eb584
📒 Files selected for processing (6)
integration_tests/dbt_project/macros/schema_utils/list_schemas.sqlintegration_tests/dbt_project/macros/schema_utils/schema_exists.sqlmacros/edr/system/system_utils/empty_table.sqlmacros/utils/cross_db_utils/sqlserver_tsql_dispatch.sqlmacros/utils/table_operations/create_temp_table.sqlmacros/utils/table_operations/replace_table_data.sql
Remove the central sqlserver_tsql_dispatch.sql file and add sqlserver__ delegates beside each fabric__ implementation so future developers can see SQL Server behavior in context. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
macros/utils/data_types/data_type.sql (1)
174-176: ⚡ Quick winInconsistent return pattern with other sqlserver delegates in this file.
Lines 15-17, 59-61, and 100-102 use
{% do return(elementary.fabric__...) %}, but this macro uses{{ elementary.fabric__... }}without an explicit return statement. While both patterns work, consistency within the same batch of additions improves maintainability.Align with the return pattern used by other sqlserver delegates in this file
{% macro sqlserver__edr_type_timestamp() %} - {{ elementary.fabric__edr_type_timestamp() }} + {% do return(elementary.fabric__edr_type_timestamp()) %} {% endmacro %}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macros/utils/data_types/data_type.sql` around lines 174 - 176, The macro sqlserver__edr_type_timestamp currently renders the delegate function with {{ elementary.fabric__edr_type_timestamp() }}; change it to follow the file's consistent return pattern by using a Jinja2 explicit return (e.g., {% do return(elementary.fabric__edr_type_timestamp()) %}) inside the sqlserver__edr_type_timestamp macro so it matches the other sqlserver delegates like the ones at lines using {% do return(elementary.fabric__...) %}.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macros/edr/data_monitoring/schema_changes/get_columns_changes_query.sql`:
- Around line 317-323: The macro sqlserver__get_column_changes_from_baseline_cur
currently calls elementary.fabric__get_column_changes_from_baseline_cur using a
`{% do %}` which discards its return value; change that call to use `{{
elementary.fabric__get_column_changes_from_baseline_cur(...) }}` so the SELECT
SQL returned by the fabric macro is emitted as output (update the call inside
sqlserver__get_column_changes_from_baseline_cur to use `{{ }}` instead of `{% do
%}`).
In `@macros/utils/cross_db_utils/incremental_strategy.sql`:
- Around line 27-29: The sqlserver__get_default_incremental_strategy macro
currently delegates to fabric__get_default_incremental_strategy (which returns
"merge") and thus overrides the adapter default; change the macro
sqlserver__get_default_incremental_strategy to return "none" so SQL Server will
use the adapter-default incremental strategy instead of forcing "merge".
---
Nitpick comments:
In `@macros/utils/data_types/data_type.sql`:
- Around line 174-176: The macro sqlserver__edr_type_timestamp currently renders
the delegate function with {{ elementary.fabric__edr_type_timestamp() }}; change
it to follow the file's consistent return pattern by using a Jinja2 explicit
return (e.g., {% do return(elementary.fabric__edr_type_timestamp()) %}) inside
the sqlserver__edr_type_timestamp macro so it matches the other sqlserver
delegates like the ones at lines using {% do return(elementary.fabric__...) %}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f60fc786-c5d6-4001-9367-21362790ca33
📒 Files selected for processing (34)
macros/edr/data_monitoring/monitors/column_numeric_monitors.sqlmacros/edr/data_monitoring/monitors/column_string_monitors.sqlmacros/edr/data_monitoring/monitors_query/get_latest_full_refresh.sqlmacros/edr/data_monitoring/monitors_query/table_monitoring_query.sqlmacros/edr/data_monitoring/schema_changes/get_columns_changes_query.sqlmacros/edr/materializations/test/failed_row_count.sqlmacros/edr/materializations/test/test.sqlmacros/edr/system/system_utils/buckets_cte.sqlmacros/edr/system/system_utils/empty_table.sqlmacros/edr/system/system_utils/full_names.sqlmacros/edr/system/system_utils/get_config_var.sqlmacros/edr/tests/test_utils/get_anomaly_query.sqlmacros/utils/cross_db_utils/concat.sqlmacros/utils/cross_db_utils/current_timestamp.sqlmacros/utils/cross_db_utils/date_trunc.sqlmacros/utils/cross_db_utils/dateadd.sqlmacros/utils/cross_db_utils/datediff.sqlmacros/utils/cross_db_utils/day_of_week.sqlmacros/utils/cross_db_utils/hour_of_day.sqlmacros/utils/cross_db_utils/hour_of_week.sqlmacros/utils/cross_db_utils/incremental_strategy.sqlmacros/utils/cross_db_utils/multi_value_in.sqlmacros/utils/cross_db_utils/time_trunc.sqlmacros/utils/cross_db_utils/timeadd.sqlmacros/utils/cross_db_utils/to_char.sqlmacros/utils/data_types/data_type.sqlmacros/utils/data_types/data_type_list.sqlmacros/utils/data_types/try_cast_column_to_timestamp.sqlmacros/utils/table_operations/create_temp_table.sqlmacros/utils/table_operations/get_relation_max_length.sqlmacros/utils/table_operations/insert_as_select.sqlmacros/utils/table_operations/insert_rows.sqlmacros/utils/table_operations/make_temp_relation.sqlmacros/utils/table_operations/replace_table_data.sql
✅ Files skipped from review due to trivial changes (1)
- macros/edr/data_monitoring/monitors_query/get_latest_full_refresh.sql
Comment on lines
+27
to
+29
| {%- macro sqlserver__get_default_incremental_strategy() %} | ||
| {% do return(elementary.fabric__get_default_incremental_strategy()) %} | ||
| {% endmacro %} |
There was a problem hiding this comment.
Keep SQL Server on adapter-default incremental strategy.
Line 28 delegates SQL Server to fabric__get_default_incremental_strategy(), which returns "merge". That overrides the generic contract here (non-Athena adapters should return none to use adapter defaults) and can force unexpected merge behavior on SQL Server models.
Suggested fix
{%- macro sqlserver__get_default_incremental_strategy() %}
- {% do return(elementary.fabric__get_default_incremental_strategy()) %}
+ {% do return(none) %}
{% endmacro %}Based on learnings: In macros/utils/cross_db_utils/incremental_strategy.sql, incremental strategy should return none for all adapters except Athena.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {%- macro sqlserver__get_default_incremental_strategy() %} | |
| {% do return(elementary.fabric__get_default_incremental_strategy()) %} | |
| {% endmacro %} | |
| {%- macro sqlserver__get_default_incremental_strategy() %} | |
| {% do return(none) %} | |
| {% endmacro %} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macros/utils/cross_db_utils/incremental_strategy.sql` around lines 27 - 29,
The sqlserver__get_default_incremental_strategy macro currently delegates to
fabric__get_default_incremental_strategy (which returns "merge") and thus
overrides the adapter default; change the macro
sqlserver__get_default_incremental_strategy to return "none" so SQL Server will
use the adapter-default incremental strategy instead of forcing "merge".
Fabric implementations use {% do return(...)} but sqlserver wrappers rendered
via {{ }} dropped the return value, causing None to leak as 'None' strings
and empty sampling results.
Co-authored-by: Cursor <cursoragent@cursor.com>
haritamar
approved these changes
May 26, 2026
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
2 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.
Summary
sqlserver__*macro delegations to the existingfabric__*T-SQL implementations across ~35 macro filesInvalid column name 'True') when buildingdbt_invocations— caused bydefault__dummy_values()emitting bareTrueinstead of T-SQL-safe1sqlserver__delegate next to itsfabric__implementation (same pattern asfabricspark__) so SQL Server behavior is discoverable in contextsqlserver__delegations for integration-test schema utilitiesContext
Linear: CORE-877
After dbt-msft/dbt-sqlserver#628 removed the Fabric adapter dependency, the dispatch chain for
type: sqlserverissqlserver__ → default__— it no longer reachesfabric__macros. T-SQL-safe logic already lives infabric__*; this PR wires SQL Server to it explicitly.Each delegate follows the existing cross-adapter convention:
Dedicated
sqlserver__implementations that differ from Fabric (e.g.get_normalized_data_type,generate_elementary_profile_args,target_database) are left unchanged.Test plan
test-all-warehousesworkflow on dbt-data-reliability withsqlservermatrix jobdbt run --select elementarysucceeds on SQL Server (dbt_invocationsbuilds withoutInvalid column name 'True')Summary by CodeRabbit