#222: parse type:databricks dbt profile target - #255
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Databricks profile parsing support across validation, adapter wiring, docs, fixtures, and tests. The changes define Databricks host and HTTP-path validation, Databricks-specific ChangesDatabricks profile parsing
Sequence Diagram(s)sequenceDiagram
participant WarehouseAdapter.from_profile
participant DbtProfileTarget.model_validate
participant validate_databricks_hostname
participant validate_databricks_http_path
participant DatabricksAdapter
WarehouseAdapter.from_profile->>DbtProfileTarget.model_validate: validate type: databricks profile
DbtProfileTarget.model_validate->>validate_databricks_hostname: check host
DbtProfileTarget.model_validate->>validate_databricks_http_path: check http_path
DbtProfileTarget.model_validate-->>WarehouseAdapter.from_profile: validated Databricks fields
WarehouseAdapter.from_profile->>DatabricksAdapter: construct with host, http_path, token, catalog, schema, auth_type, client_id, client_secret
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
…leTarget (US-002, #222) Teach the unified DbtProfileTarget to parse a `type: databricks` dbt profile, mirroring the Snowflake per-type validator arm (#120): - 7 new Optional fields: host, http_path, catalog, client_id, auth_type plus the secret material token + client_secret (repr=False, like Snowflake password). `dataset` (schema alias) + `threads` stay shared; catalog is its own new field (the analogue of project / database). - _DATABRICKS_ONLY frozenset + _DATABRICKS_REQUIRED (host, http_path always) + _DATABRICKS_SUPPORTED_AUTH ({pat, oauth}, None = default PAT). - databricks arm in _validate_type_field_coherence: unsupported auth_type -> UnsupportedAuthMethodError FIRST (a bad method changes which keys are required); auth-conditional required-key collect-all (PAT needs token; oauth needs client_id + client_secret) -> IncompleteProfileError; foreign _BIGQUERY_ONLY | _SNOWFLAKE_ONLY rejection (plain ValueError); identifier hygiene on catalog/schema (strict) + host/http_path (permissive US-001 validators). - bigquery + snowflake arms extended to also reject _DATABRICKS_ONLY fields. - _DATABRICKS_DEFERRED_AUTH_REMEDIATION in errors.py (imported by name, not exported in __all__, mirroring _SNOWFLAKE_DEFERRED_AUTH_REMEDIATION). Tests: full PAT + OAuth parse, missing http_path / PAT token / oauth client creds, unknown auth_type, foreign-field rejection both directions (bq<->databricks, snowflake<->databricks), bad catalog/schema/host/http_path, secret repr redaction. The two #221 from_profile dispatch stub tests fed a project/schema-only databricks profile (now correctly rejected — project is BigQuery-only); updated to a valid full target. base.py's catalog<-profile.project mapping is left for US-003 to rewire, so _catalog stays None for now (asserted, flips in US-003).
…etector + load_profile e2e (US-003, #222) Rewire WarehouseAdapter.from_profile's databricks branch to pass the real parsed DbtProfileTarget fields (host / http_path / token / catalog + the OAuth-M2M auth fields), replacing the #221 placeholder catalog<-project mapping. Add the mandatory drift detector (StrictDatabricksModel + dbt_databricks_drift_v1_x.yml fixture) and a load_profile end-to-end fixture + test, mirroring the shipped Snowflake #120 work. Flip the transitional from_profile dispatch assertion left by US-002 to reflect the now-correct field wiring.
…pass + add connection-knob passthrough fields + test hardening
- profiles.py: required-key check treats empty/whitespace as missing (token/client_id/client_secret could bypass via is-None-only check; realistic trigger is an unset env_var('...','') rendering to '')
- profiles.py: add connect_retries/connect_timeout/connect_max_idle as accepted-but-unused shared fields (mirrors the threads precedent) so a real dbt-databricks profile parses under extra=forbid
- _sql_safety.py: fix off-by-one in http_path length comment (1-511 after slash)
- test_profiles.py: drop vacuous 'or host'/'or http_path' assertion fallback; add explicit auth_type=pat, short-catalog 'main', empty-credential rejection, and connection-knob acceptance tests
…cks profile arm - warehouse-adapters.md: new 'Databricks arm (issue #222)' subsection under Unified DbtProfileTarget (conditional auth-keyed required-set, empty-string-as-missing credential check, shared connection-knob passthrough fields); update the #221 skeleton deferral note to point at the landed work - warehouse-adapter-ops.md: document type:databricks profile parsing in the from_profile dispatch section + UnsupportedProfileTypeError row
There was a problem hiding this comment.
Pull request overview
Adds Databricks (type: databricks) support to the unified DbtProfileTarget profile parser, including permissive non-SQL validation for connection fields, auth-conditional required-key logic, and drift-detection fixtures/tests—then wires the parsed fields through WarehouseAdapter.from_profile to the existing Databricks skeleton adapter.
Changes:
- Extend
DbtProfileTargetwith Databricks connection/auth fields and adatabricksarm in the cross-field validator (including symmetric foreign-field rejection). - Add permissive validators for Databricks
hostandhttp_path, plus unit tests and end-to-endload_profilecoverage. - Add a Databricks drift detector fixture + strict mirror model, and update docs/rules/plan to reflect the new parsing behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/signalforge/warehouse/_sql_safety.py | Adds permissive Databricks hostname/http_path validators (non-SQL hygiene). |
| src/signalforge/warehouse/profiles.py | Adds Databricks fields, auth-conditional required set, and foreign-field rejection in the model validator. |
| src/signalforge/warehouse/errors.py | Adds Databricks deferred-auth remediation string for UnsupportedAuthMethodError. |
| src/signalforge/warehouse/base.py | Wires parsed Databricks fields through WarehouseAdapter.from_profile into DatabricksAdapter. |
| tests/warehouse/test_sql_safety.py | Unit tests for the new Databricks host/http_path validators (valid + garbage + error rendering). |
| tests/warehouse/test_profiles.py | Databricks parsing tests (PAT + OAuth), foreign-field rejection, drift detector, and load_profile e2e test. |
| tests/warehouse/test_databricks_stub.py | Updates/extends factory dispatch test to assert real Databricks fields are passed through. |
| tests/fixtures/profiles/dbt_databricks_drift_v1_x.yml | Adds Databricks drift-detector fixture for strict mirror validation. |
| tests/fixtures/profiles/databricks_pat.yml | Adds minimal Databricks PAT profiles.yml fixture for load_profile e2e test. |
| docs/warehouse-adapter-ops.md | Documents Databricks profile parsing and updates supported profile types. |
| .claude/rules/warehouse-adapters.md | Records the new Databricks arm conventions/patterns for future work. |
| plans/super/222-databricks-profile.md | Adds the super plan describing scope/decisions/tests for #222. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (1)
src/signalforge/warehouse/profiles.py (1)
279-293: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty-string
auth_typeis treated as unsupported, not as default PAT.The required-key block deliberately normalizes empty/whitespace credentials to "missing" to handle an unset
env_var(..., '')render. Anauth_typerendered to""from the same templating pattern instead hits Line 279 ("" is not Noneand"" not in _DATABRICKS_SUPPORTED_AUTH) and raisesUnsupportedAuthMethodError(method="")rather than defaulting to PAT. The error is still loud and lists the supported set, so this isn't a correctness bug — just an asymmetry with the credential handling you explicitly cared about. Consider normalizing a blankauth_typetoNonefor consistency.♻️ Optional normalization
- if self.auth_type is not None and self.auth_type not in _DATABRICKS_SUPPORTED_AUTH: + auth_type = (self.auth_type or "").strip() or None + if auth_type is not None and auth_type not in _DATABRICKS_SUPPORTED_AUTH: raise UnsupportedAuthMethodError( - method=self.auth_type, + method=self.auth_type, remediation=_DATABRICKS_DEFERRED_AUTH_REMEDIATION, )(Use the normalized value in the
== "oauth"branch below too.)🤖 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 `@src/signalforge/warehouse/profiles.py` around lines 279 - 293, The auth_type handling in the Databricks profile validation treats an empty string as unsupported instead of falling back to default PAT like the credential fields do. Normalize blank or whitespace-only auth_type to None before the _DATABRICKS_SUPPORTED_AUTH check in profiles.py, and use that normalized value in the subsequent oauth vs PAT required-key branch so the behavior is consistent with the empty-string handling for token/client fields.
🤖 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 `@docs/warehouse-adapter-ops.md`:
- Line 800: The docs row and UnsupportedProfileTypeError.default_remediation are
out of sync about supported profile types. Update the runtime remediation text
in UnsupportedProfileTypeError (or revise the warehouse docs table) so the
supported/unsupported dbt profile types match exactly, using the existing
UnsupportedProfileTypeError symbol in src/signalforge/warehouse/errors.py as the
source of truth.
- Around line 68-75: The WarehouseAdapter documentation overstates the
Databricks skeleton contract in the section describing
WarehouseAdapter.from_profile and the databricks adapter. Update the wording so
it is precise: only sample_rows, column_stats, and run_test_sql still raise
NotImplementedError for Databricks, while the other warehouse methods already
inherit typed *NotSupportedError defaults from the ABC. Keep the
dispatch/remediation description for profile.type accurate and align the adapter
behavior summary with the actual Databricks methods.
In `@plans/super/222-databricks-profile.md`:
- Line 34: The Databricks profile contract is inconsistent because the
documentation marks schema as required while the validator/tests still treat it
as the shared dataset alias. Update the docs to remove schema from the
required-key list, or if schema must be mandatory, align the implementation by
updating _DATABRICKS_REQUIRED and the related missing-key tests so the contract
matches the code.
---
Nitpick comments:
In `@src/signalforge/warehouse/profiles.py`:
- Around line 279-293: The auth_type handling in the Databricks profile
validation treats an empty string as unsupported instead of falling back to
default PAT like the credential fields do. Normalize blank or whitespace-only
auth_type to None before the _DATABRICKS_SUPPORTED_AUTH check in profiles.py,
and use that normalized value in the subsequent oauth vs PAT required-key branch
so the behavior is consistent with the empty-string handling for token/client
fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43cdfe85-64fe-4d0c-a73d-5719744a25ea
📒 Files selected for processing (12)
.claude/rules/warehouse-adapters.mddocs/warehouse-adapter-ops.mdplans/super/222-databricks-profile.mdsrc/signalforge/warehouse/_sql_safety.pysrc/signalforge/warehouse/base.pysrc/signalforge/warehouse/errors.pysrc/signalforge/warehouse/profiles.pytests/fixtures/profiles/databricks_pat.ymltests/fixtures/profiles/dbt_databricks_drift_v1_x.ymltests/warehouse/test_databricks_stub.pytests/warehouse/test_profiles.pytests/warehouse/test_sql_safety.py
wjduenow
changed the title
- errors.py: UnsupportedProfileTypeError docstring + default_remediation now name postgres/snowflake/databricks with correct version tracks (was stale 'bigquery only … v0.2'); source of truth for the docs row - warehouse-adapter-ops.md: precise NotImplementedError-vs-*NotSupportedError scope for the Databricks skeleton; error-table row synced to the runtime remediation with correct v0.2/v0.x labels - plan: clarify dbt-requires-schema vs SignalForge's narrower required-VALIDATED set (schema/catalog are parse-optional, not in _DATABRICKS_REQUIRED)
Owner
Author
PR Review SummaryAll five review threads addressed (fixed in Fixed (5 items)
False Positives (0 items)All threads resolved. |
This was referenced Jun 29, 2026
Merged
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
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
Parse a
type: databricksdbt profile target into the unifiedDbtProfileTarget(single model, discriminated-by-validator — not a union). Per-warehouse twin of the shipped #120 (Snowflake); builds on the #221 skeleton. Closes #222.Changes
DbtProfileTargetfields:host,http_path,token(repr=False),catalog,client_id,client_secret(repr=False),auth_type;_DATABRICKS_ONLYfrozenset.databricksarm in the cross-field validator: auth-conditional required-set (PAT→token; OAuth→client_id+client_secret; auth-validity checked first), bidirectional foreign-field rejection, identifier hygiene. Empty/whitespace credentials count as missing (an unsetenv_var('…','')→""no longer slips through).validate_databricks_hostname/validate_databricks_http_path(mirrorvalidate_snowflake_account).connect_retries/connect_timeout/connect_max_idleadded as accepted-but-unused (likethreads) so real dbt-databricks profiles parse underextra="forbid".from_profilewires every field (lazy import); drift detector (StrictDatabricksModel+dbt_databricks_drift_v1_x.yml);load_profilee2e fixture + test.Testing
uv sync --dev && ruff check && ruff format --check && pyright && pytest— green (4191 passed, 6 skipped; pyright 0 errors). CI green on 3.11/3.12/3.13.Compounding Update
.claude/rules/warehouse-adapters.md§ "Databricks arm (issue Databricks: parse 'type: databricks' dbt profile target #222)" — conditional auth-keyed required-set, empty-string-as-missing, shared connection-knob fields.docs/warehouse-adapter-ops.md—type: databricksparsing + dispatch + error-table row.Plan:
plans/super/222-databricks-profile.md.Summary by CodeRabbit
New Features
Bug Fixes