Skip to content

fix: add explicit sqlserver__ dispatches in elementary_cli (CORE-877)#2241

Merged
elazarlachkar merged 3 commits into
masterfrom
cursor/core-877-sqlserver-cli-dispatch
May 27, 2026
Merged

fix: add explicit sqlserver__ dispatches in elementary_cli (CORE-877)#2241
elazarlachkar merged 3 commits into
masterfrom
cursor/core-877-sqlserver-cli-dispatch

Conversation

@elazarlachkar
Copy link
Copy Markdown
Contributor

@elazarlachkar elazarlachkar commented May 27, 2026

Summary

  • Add sqlserver__get_test_results delegating to fabric__get_test_results to avoid nested CTE errors on T-SQL during edr monitor report
  • Add sqlserver__get_adapter_unique_id returning target.server (sqlserver profiles use server, not host)
  • Fix misleading comment that assumed sqlserver inherits fabric dispatch (same issue fixed in dbt-data-reliability Ele 1068 supperssion in cli #1013)

Context

dbt-sqlserver no longer declares dbt-fabric as a dispatch parent (dbt-msft/dbt-sqlserver#628), so fabric__ macros in the elementary_cli namespace are not resolved for sqlserver targets. Warehouse CI on master passes 13/14 adapters; only sqlserver fails at the report step with Incorrect syntax near the keyword 'with'.

Audited all elementary_cli macros — only these two had fabric__ implementations without explicit sqlserver__ delegates. Other T-SQL-aware macros (e.g. test_conn) use elementary.is_tsql() in default__.

Test plan

  • Warehouse CI passes for sqlserver (especially edr monitor report)
  • Fabric CI still passes (unchanged behavior)

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added SQL Server-specific system identification to provide a stable adapter unique ID.
    • Routed SQL Server test-result retrieval through the fabric implementation for consistent behavior.
  • Documentation

    • Clarified notes about T-SQL nested-CTE limitations and the need to materialize intermediate results.

Review Change Stack

dbt-sqlserver no longer includes fabric in its adapter dispatch chain,
so fabric__ macros in elementary_cli are not picked up for sqlserver targets.
Add sqlserver__ delegates for get_test_results and get_adapter_unique_id.

Co-authored-by: Cursor <cursoragent@cursor.com>
@linear
Copy link
Copy Markdown

linear Bot commented May 27, 2026

CORE-877

@github-actions
Copy link
Copy Markdown
Contributor

👋 @elazarlachkar
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f514d9e4-1786-4de7-a55d-ba28c1f8e55c

📥 Commits

Reviewing files that changed from the base of the PR and between 2caa016 and ee6304d.

📒 Files selected for processing (1)
  • elementary/monitor/dbt_project/macros/get_test_results.sql
💤 Files with no reviewable changes (1)
  • elementary/monitor/dbt_project/macros/get_test_results.sql

📝 Walkthrough

Walkthrough

Adds SQL Server adapter-specific dbt macros: sqlserver__get_adapter_unique_id() (returns target.server) and sqlserver__get_test_results() (delegates to the fabric implementation); also clarifies a comment in fabric__get_test_results about T-SQL nested-CTE limitations.

Changes

SQL Server Adapter Support

Layer / File(s) Summary
SQL Server adapter dispatcher macros
elementary/monitor/dbt_project/macros/get_adapter_type_and_unique_id.sql, elementary/monitor/dbt_project/macros/get_test_results.sql
Added sqlserver__get_adapter_unique_id() macro returning target.server; updated fabric__get_test_results comment about T-SQL CTE behavior; added sqlserver__get_test_results() delegating parameters to elementary_cli.fabric__get_test_results().

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I nibble on SQL with a cheerful cheer,
target.server shines and the path is clear,
Tests routed gently through fabric's gate,
Small macros hop in to set things straight,
A tiny rabbit dance for the SQL state.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding explicit sqlserver__ dispatches to address SQL Server compatibility issues in elementary_cli.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/core-877-sqlserver-cli-dispatch

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread elementary/monitor/dbt_project/macros/get_test_results.sql Outdated
Address review feedback — the explicit sqlserver__ macro makes the
delegation path self-evident.

Co-authored-by: Cursor <cursoragent@cursor.com>
@elazarlachkar elazarlachkar enabled auto-merge May 27, 2026 08:38
@elazarlachkar elazarlachkar disabled auto-merge May 27, 2026 09:27
@elazarlachkar elazarlachkar merged commit 1a73090 into master May 27, 2026
25 of 26 checks passed
@elazarlachkar elazarlachkar deleted the cursor/core-877-sqlserver-cli-dispatch branch May 27, 2026 11:13
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.

2 participants