feat(warehouse): #221 Databricks adapter skeleton - #254
Merged
Conversation
… DATABRICKS_DIALECT + SDK shim) Stands up the warehouse-agnostic seam for a FOURTH vendor (after BigQuery, the Postgres stub, and Snowflake), mirroring the Snowflake skeleton #119. - DatabricksAdapter: captures host/http_path/token/catalog/schema + forward-compat OAuth-M2M params (no eager connect); __repr__ redacts token/schema/client_secret; sample_rows/column_stats/run_test_sql raise NotImplementedError naming epic #219; materialise_sample/estimate_query_bytes/get_row_count/run_stats_query inherit the ABC typed degrade; fail-soft __exit__ cleanup. - _databricks_client.py: one-shim-per-vendor SDK seam — duck-typed connection/cursor protocols, lazy make_real_client (PAT auth), minimal map_databricks_exception stub (auth -> WarehouseAuthError; full taxonomy in #226). Confinement test pins it. - DATABRICKS_DIALECT: backtick quoting, identifier_case=lower (Unity Catalog), QUALIFY, 64-bit xxhash64 sampling hash, per-component qualified quoting, Spark date-arithmetic fields. Provisional values certified by the #223 sqlglot parse-guard + #226 live. - from_profile dispatches type: databricks (maps catalog<-project, schema<-dataset; real host/http_path/token fields land with profile parsing in #222). - [databricks] optional extra + dev-group mirror + databricks pytest marker + addopts exclusion; uv.lock updated additively. - Repoint test_from_profile_raises_for_unknown_type to redshift (databricks now dispatches).
📝 WalkthroughWalkthroughAdds Databricks warehouse support as a v0.x skeleton: new dialect constants and exports, a Databricks adapter with lazy client creation and cleanup, ChangesDatabricks warehouse support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 `@src/signalforge/warehouse/models.py`:
- Line 314: The sample hash expression in DATABRICKS_DIALECT is using
ABS(xxhash64(...)), which can still produce a negative value for Long.MinValue
and skew deterministic sampling. Update the sample_row_hash_expr definition in
the relevant warehouse model to use a non-overflowing normalization approach
instead of ABS, and keep the change localized to the sample hash logic so the
sampling predicate remains stable and unbiased.
🪄 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: 942b2278-8843-4afd-9bff-3d6bfe65c21a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
pyproject.tomlsrc/signalforge/warehouse/__init__.pysrc/signalforge/warehouse/adapters/_databricks_client.pysrc/signalforge/warehouse/adapters/databricks.pysrc/signalforge/warehouse/base.pysrc/signalforge/warehouse/models.pytests/warehouse/test_base.pytests/warehouse/test_databricks_client_confinement.pytests/warehouse/test_databricks_stub.py
Compounding update for the #221 closeout. Adds the Databricks skeleton precedent (4th adapter, mirrors Snowflake #119), the provisional-dialect/certification note (values certified by #223 sqlglot parse-guard + #226 live, not snapshot equality), and the load-bearing TableRef.project length gotcha for #224 (GCP project-id 6-30 char rule blocks Unity Catalog catalog names like 'main'). Also brings the adapters/ file tree current (snowflake + databricks + their shims).
… sample hash CodeRabbit (PR #254): ABS(xxhash64(...)) can stay negative on Long.MIN_VALUE in Spark non-ANSI mode, skewing MOD(<expr>, bucket) < 1. Replace with a sign-bit mask (& 9223372036854775807 = Long.MAX_VALUE) — always non-negative, no overflow, uniform, and keeps the renderer's MOD wrapper correct. Pinned by a no-ABS / mask assertion in test_databricks_dialect_values; docstring + rules note updated.
Owner
Author
PR Review SummaryFixed (1 item)
Fix: replaced This value is still provisional at the skeleton stage (the compiler isn't invoked for a Databricks profile yet) and will be executed/certified by the #223 False Positives (0 items)None. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/rules/warehouse-adapters.md (1)
37-37: 🎯 Functional Correctness | 🟠 MajorKeep Unity Catalog catalogs out of
TableRef.project.TableRef.projectstill follows GCP project-id validation, so a short Databricks catalog likemainwill fail if the#224sampling path threads it throughTableRef. Keep catalog on the Databricks field split or relax the shared validator before three-part refs land.🤖 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 @.claude/rules/warehouse-adapters.md at line 37, Keep Unity Catalog catalog names out of TableRef.project, because TableRef still validates project as a GCP project id and short Databricks catalogs like main will fail construction. Update the Databricks sampling path that builds three-part refs (the code that threads quote_qualified_per_component into TableRef) so the catalog is stored on the Databricks-specific field split instead of project, or relax validate_project_id only for the Databricks dialect. Verify the TableRef construction and any helper that maps catalog.schema.table avoid passing catalog through project.
🤖 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.
Outside diff comments:
In @.claude/rules/warehouse-adapters.md:
- Line 37: Keep Unity Catalog catalog names out of TableRef.project, because
TableRef still validates project as a GCP project id and short Databricks
catalogs like main will fail construction. Update the Databricks sampling path
that builds three-part refs (the code that threads quote_qualified_per_component
into TableRef) so the catalog is stored on the Databricks-specific field split
instead of project, or relax validate_project_id only for the Databricks
dialect. Verify the TableRef construction and any helper that maps
catalog.schema.table avoid passing catalog through project.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94ef1a63-3a90-4cf6-bfb8-69daf4d3adaa
📒 Files selected for processing (3)
.claude/rules/warehouse-adapters.mdsrc/signalforge/warehouse/models.pytests/warehouse/test_databricks_stub.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/signalforge/warehouse/models.py
- tests/warehouse/test_databricks_stub.py
This was referenced Jun 25, 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
1 participant
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.
Part of epic #219 (Databricks warehouse adapter); second child after the #220 test-environment spike. Models on the Snowflake skeleton #119.
Stands up the warehouse-agnostic seam (Architectural Commitment #3) for a fourth vendor — after BigQuery, the Postgres stub, and Snowflake — with everything degrading gracefully, so the ABC +
from_profilefactory path is exercised by Databricks immediately. No live connectivity yet (that's #224+).What's here
DatabricksAdapter(adapters/databricks.py) — captureshost/http_path/token/catalog/schema+ forward-compat OAuth-M2M params (auth_type/client_id/client_secret), no eager connect;__repr__redactstoken/schema/client_secret;sample_rows/column_stats/run_test_sqlraiseNotImplementedErrornaming epic Epic: Databricks warehouse adapter #219;materialise_sample/estimate_query_bytes/get_row_count/run_stats_queryinherit the ABC typed degrade; fail-soft__exit__cleanup.adapters/_databricks_client.py) — one-shim-per-vendor seam: duck-typed connection/cursor protocols, lazymake_real_client(PAT auth), minimalmap_databricks_exceptionstub (auth →WarehouseAuthError; full taxonomy in Databricks: test harness + gated live e2e + ops docs #226). Confinement scan pins it.DATABRICKS_DIALECT(models.py) — backtick quoting,identifier_case=lower(Unity Catalog),QUALIFY, 64-bitxxhash64sampling hash (Spark's barehash(*)is Murmur3-32), per-component qualified quoting, Spark date-arithmetic/percentile fields. Provisional values certified by the Databricks: prune compiler emits valid Databricks SQL from DATABRICKS_DIALECT #223 sqlglot parse-guard + Databricks: test harness + gated live e2e + ops docs #226 live — the prune compiler is never invoked for a Databricks profile at this stage.from_profiledispatchestype: databricks(mapscatalog<-project,schema<-dataset; real host/http_path/token fields land with profile parsing in Databricks: parse 'type: databricks' dbt profile target #222, mirroring the Snowflake feat: SnowflakeAdapter skeleton — factory dispatch + Dialect + _snowflake_client.py shim #119→feat: parse Snowflake target in dbt profiles.yml (DbtProfileTarget type=snowflake) #120 deferral).[databricks]optional extra + dev-group mirror (lightweight + not constraints-pinned, so unlike[airflow]the normal mirror applies) +databrickspytest marker + addopts exclusion;uv.lockupdated additively.Guardrails honoured (
.claude/rules/warehouse-adapters.md)databricks-sql-connectortype-ignore confined to_databricks_client.py(paired confinement test).make_real_client/ thefrom_profilebranch —import signalforge.warehousestays SDK-free (subprocess-asserted).*NotSupportedErrordefaults — no bareNotImplementedErrorescaping the orchestrator for the degrade-default methods.Validation
uv sync --dev && uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run pytest— green (pyright 0 errors; 4142 passed, 97% cov).Collateral
test_from_profile_raises_for_unknown_typetoredshift(databricks now dispatches).Acceptance (per #221)
Adapter constructs without a live connection;
from_profiledispatches adatabrickstarget to it;dialect()returns the constant; degrade methods raise their typed errors;__repr__redaction test passes; confinement-scan scaffolding present. ✅Summary by CodeRabbit
databricksprofile type with automatic adapter dispatch.redshift.