feat(wren-launcher) support MSSQL data source for dbt-tools #1887
Conversation
WalkthroughAdds Microsoft SQL Server (sqlserver/MSSQL) support across DBT parsing, conversion, and tests: new WrenMSSQLDataSource type, conversion/mapping logic, profiles parsing for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (input)
participant From as FromDbtProfiles
participant Analyzer as ProfilesAnalyzer
participant Converter
participant WrenDS as WrenMSSQLDataSource
participant Out as wren-datasource.json
Dev->>From: provide dbt profiles (type: sqlserver)
From->>Analyzer: parse connection map
Analyzer-->>From: DbtConnection (Host, Server, Port, ...)
From->>Converter: convertConnectionToDataSource(conn)
Converter->>Converter: select "sqlserver" branch
alt opts.UsedByContainer == true
Converter->>WrenDS: host = handleLocalhostForContainer(conn.Server)
else
Converter->>WrenDS: host = conn.Server
end
Converter->>WrenDS: set port (default 1433), tds_version, driver, kwargs
Converter->>Out: write wren-datasource.json
Out-->>Dev: datasource file emitted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-13T05:17:30.180ZApplied to files:
📚 Learning: 2025-07-09T02:43:20.433ZApplied to files:
🧬 Code graph analysis (1)wren-launcher/commands/dbt/data_source.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
wren-launcher/commands/dbt/profiles.go (1)
19-19: Add MSSQL server field: LGTM, consider clarifying precedence with Host.The addition of Server aligns with dbt-sqlserver’s configuration. Downstream, ensure MSSQL uses Server but gracefully falls back to Host if Server is empty to handle misconfigured profiles. I'll propose the fallback change in convertToMSSQLDataSource.
wren-launcher/commands/dbt/data_source_test.go (1)
196-261: MSSQL profile conversion test: solid coverage of defaults and field mapping.This verifies Server→Host mapping, default MSSQL settings, and the shape of the data source. Consider adding:
- Validation tests for MSSQL (e.g., non-numeric port, missing host, missing password).
- Type mapping tests (e.g., tinyint, datetimeoffset) to pin behavior.
I can draft these tests if helpful.
wren-launcher/commands/dbt/converter.go (1)
142-161: MSSQL export: verify expected JSON contract for port and consider instance-name localhost handling.
- The MSSQL port is emitted as a string, while Postgres emits an integer. If the consumer of wren-datasource.json differentiates these types, confirm the "mssql" port must be a string. If not mandated, consider normalizing the type to minimize schema drift.
- For server values like "localhost\SQLEXPRESS" or "(local)", the simple equality check in handleLocalhostForContainer won’t rewrite localhost. Suggest enhancing it to handle instance suffixes and aliases.
If acceptable, update handleLocalhostForContainer to handle instance names:
func handleLocalhostForContainer(host string) string { base := host instance := "" if idx := strings.Index(host, `\`); idx != -1 { base = host[:idx] instance = host[idx:] // includes backslash } if base == "." || strings.EqualFold(base, "(local)") { base = "localhost" } if base == "localhost" || base == "127.0.0.1" { return "host.docker.internal" + instance } return host }wren-launcher/commands/dbt/data_source.go (1)
272-296: Validation inconsistency: MSSQL requires password, Postgres does not.If Wren allows password-less connections (e.g., integrated auth, env-injected credentials), requiring Password here but not for Postgres is inconsistent and may block valid configurations. Decide one policy and apply consistently.
Options:
- Allow empty Password for MSSQL (align with Postgres).
- Require Password across both MSSQL and Postgres.
Do you want a follow-up patch either way?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wren-launcher/commands/dbt/converter.go(1 hunks)wren-launcher/commands/dbt/data_source.go(4 hunks)wren-launcher/commands/dbt/data_source_test.go(1 hunks)wren-launcher/commands/dbt/profiles.go(1 hunks)wren-launcher/commands/dbt/profiles_analyzer.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T02:43:20.433Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:50-53
Timestamp: 2025-07-09T02:43:20.433Z
Learning: In wren-launcher/commands/dbt/data_source.go, when encountering unsupported database types in convertConnectionToDataSource function, the user prefers to log a warning and continue processing instead of failing. The function should return nil, nil to allow processing to continue.
Applied to files:
wren-launcher/commands/dbt/converter.gowren-launcher/commands/dbt/data_source.go
🧬 Code Graph Analysis (3)
wren-launcher/commands/dbt/data_source_test.go (2)
wren-launcher/commands/dbt/profiles.go (3)
DbtProfiles(4-7)DbtProfile(10-13)DbtConnection(16-40)wren-launcher/commands/dbt/data_source.go (2)
FromDbtProfiles(59-81)WrenMSSQLDataSource(257-266)
wren-launcher/commands/dbt/converter.go (1)
wren-launcher/commands/dbt/data_source.go (1)
WrenMSSQLDataSource(257-266)
wren-launcher/commands/dbt/data_source.go (1)
wren-launcher/commands/dbt/profiles.go (1)
DbtConnection(16-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-launcher/commands/dbt/data_source.go (3)
6-6: Import strconv: LGTM.Needed for port conversions in MSSQL paths.
14-29: Type constants: LGTM.The expanded set covers MSSQL mappings well and improves consistency across MapType implementations.
90-92: Add 'sqlserver' support in converter: aligns with prior preference to skip unsupported types.The new case integrates cleanly. The default branch continues to warn and skip unsupported types, matching the user preference noted in prior learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wren-launcher/commands/dbt/data_source.go (2)
118-141: MSSQL DS construction: sensible defaults + host fallback (good).
Defaulting port to 1433, TDS 8.0, the ODBC 18 driver, and TrustServerCertificate are pragmatic. The host fallback to conn.Host when Server is empty addresses earlier feedback.Please verify that downstream consumers expect TrustServerCertificate to be a string ("YES"/"NO") rather than a boolean. Run:
#!/bin/bash set -e echo "Search TrustServerCertificate usage and Kwargs consumption:" rg -nI 'TrustServerCertificate' -A 3 rg -nI 'Kwargs' -A 5 | sed -n '1,200p' echo echo "Search for WrenMSSQLDataSource consumer code paths:" rg -nI 'WrenMSSQLDataSource' -A 20
303-339: Correct MSSQL type mappings: tinyint is numeric; time is time-of-day.
- tinyint (0–255) should not map to boolean.
- time should not map to interval; fall back to the original type until a dedicated mapping exists.
Apply this diff:
func (ds *WrenMSSQLDataSource) MapType(sourceType string) string { // This method is not used in WrenMSSQLDataSource, but required by DataSource interface switch strings.ToLower(sourceType) { @@ - case "bit", "tinyint": - return booleanType + case "bit": + return booleanType + case "tinyint": + return smallintType @@ - case "time": - return intervalType + case "time": + // No dedicated Wren type for time-of-day; keep original + return strings.ToLower(sourceType) case "datetimeoffset": return timestamptzType case "json": return jsonType default: return strings.ToLower(sourceType) } }I can add/extend unit tests to cover:
- MapType("tinyint") -> smallintType
- MapType("time") -> "time"
- Host fallback when Server is empty
Would you like me to draft those tests?
🧹 Nitpick comments (2)
wren-launcher/commands/dbt/data_source.go (2)
90-91: Also accept “mssql” alias in addition to “sqlserver”.
Some profiles/plugins use “mssql”. Recognizing both improves resilience without behavior change.Apply this diff:
- case "sqlserver": + case "sqlserver", "mssql": return convertToMSSQLDataSource(conn)
277-301: Validation is solid; minor hardening: trim whitespace.
Guard against accidental whitespace-only values without mutating state.Apply this diff:
func (ds *WrenMSSQLDataSource) Validate() error { - if ds.Host == "" { + if strings.TrimSpace(ds.Host) == "" { return fmt.Errorf("host cannot be empty") } - if ds.Database == "" { + if strings.TrimSpace(ds.Database) == "" { return fmt.Errorf("database cannot be empty") } - if ds.User == "" { + if strings.TrimSpace(ds.User) == "" { return fmt.Errorf("user cannot be empty") } - if ds.Port == "" { + if strings.TrimSpace(ds.Port) == "" { return fmt.Errorf("port must be specified") } port, err := strconv.Atoi(ds.Port) @@ - if ds.Password == "" { + if strings.TrimSpace(ds.Password) == "" { return fmt.Errorf("password cannot be empty") } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-launcher/commands/dbt/data_source.go(4 hunks)wren-launcher/commands/dbt/profiles_analyzer.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-launcher/commands/dbt/profiles_analyzer.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-09T02:43:20.433Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:50-53
Timestamp: 2025-07-09T02:43:20.433Z
Learning: In wren-launcher/commands/dbt/data_source.go, when encountering unsupported database types in convertConnectionToDataSource function, the user prefers to log a warning and continue processing instead of failing. The function should return nil, nil to allow processing to continue.
Applied to files:
wren-launcher/commands/dbt/data_source.go
📚 Learning: 2025-07-09T02:43:07.560Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:154-156
Timestamp: 2025-07-09T02:43:07.560Z
Learning: In the WrenAI codebase, the MapType methods for data sources should return the original type name for unknown types, rather than a default fallback like "string". This maintains consistency across all data source implementations.
Applied to files:
wren-launcher/commands/dbt/data_source.go
🧬 Code Graph Analysis (1)
wren-launcher/commands/dbt/data_source.go (1)
wren-launcher/commands/dbt/profiles.go (1)
DbtConnection(16-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: fmt-and-test
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-launcher/commands/dbt/data_source.go (3)
6-6: Import of strconv is appropriate for port handling.
Used for Port string conversion and validation; no issues spotted.
14-28: Type constants look consistent and broadly useful.
Centralized constants align with existing mappings and support MSSQL additions.
262-271: WrenMSSQLDataSource struct looks correct.
JSON field names and types match construction and validation; no structural issues noted.
goldmedal
force-pushed
the
chore/dbt-mssql
branch
from
48b7287 to
856ac3e
Compare
Summary by CodeRabbit
New Features
Tests