fix(databricks): prefer python-connector spec over legacy Hive spec for unrecognized drivers

[eschutho]

SUMMARY

Fixes TypeError: Cursor.execute() got an unexpected keyword argument 'async'
raised from DatabaseRestApi.table_extra_metadata for certain Databricks
connections. Reproduces on every table-metadata lookup (SQL Lab / dataset
panel) for any affected connection.

Root cause. get_engine_spec(backend, driver) matches a driver string
exactly first; if no registered spec's drivers dict matches, it falls back to
the first backend-matching spec in module-definition order. For
engine="databricks" there are four concrete specs. DatabricksHiveEngineSpec
was defined first, so any stored Databricks sqlalchemy_uri whose driver suffix
doesn't exactly match one of the four registered driver keys (e.g. a URI saved
before the databricks-sql-python key existed, or a hand-written/legacy suffix)
fell through to this ambiguous fallback and was resolved as
DatabricksHiveEngineSpec — even though the URL still resolves to and actually
connects via the databricks-sql-connector package's
databricks.sql.client.Cursor.

DatabricksHiveEngineSpec inherits HiveEngineSpec.execute(), which
unconditionally calls cursor.execute(query, **{"async": async_}). A real
pyhive cursor swallows the async kwarg, but databricks.sql.client.Cursor
has a fixed signature with no **kwargs, so it raises the TypeError. This is
reached from PrestoEngineSpec.get_create_view (inherited by Hive) on every
get_extra_table_metadata call.

Call chain: databases/api.py:table_extra_metadata -> presto.py:get_extra_table_metadata
-> presto.py:get_create_view -> hive.py:execute -> databricks cursor.

Precedent. This is the same crash as #24786 (2024, "Hive
async kwarg unexpected keyword when connecting to Databricks Unity Catalog"),
via a different call path (SQL Lab query execution). The naive fix there
(PR #24984 — remove the async kwarg from HiveEngineSpec.execute) was closed
unmerged because it would break async_/query-cancellation semantics for genuine
pyhive/Presto connections. There is also an explicit test,
test_presto.py::test_get_create_view_propagates_other_errors, asserting that
non-DatabaseError exceptions propagate out of get_create_view, so widening
that except clause is intentionally off-limits too.

The fix. Rather than touch the shared Hive/Presto code paths, this reorders
the concrete Databricks engine-spec class definitions so the general-purpose
DatabricksPythonConnectorEngineSpec is defined first and therefore wins the
ambiguous-driver fallback, with the legacy Native/ODBC/Hive specs defined after
it. load_engine_specs() iterates module.__dict__, which preserves
definition order, so the fallback now lands on the modern connector — which uses
databricks.sql.client.Cursor directly and never goes through
HiveEngineSpec.execute(). Pure relocation of class definitions plus comments
documenting the ordering invariant; no logic changes.

Note: moving only the Hive class (as first scoped) is insufficient —
DatabricksODBCEngineSpec and DatabricksNativeEngineSpec sit between Hive and
the Python connector in the file, so the fallback would otherwise land on ODBC.
The connector must be the first concrete Databricks spec; the change reorders
accordingly.

TESTING INSTRUCTIONS

• New unit test test_databricks.py::test_get_engine_spec_unrecognized_driver_prefers_python_connector
  asserts get_engine_spec("databricks", "some-unrecognized-driver-name") now
  returns DatabricksPythonConnectorEngineSpec, and that exact matches still
  resolve correctly (pyhive -> Hive, databricks-sql-python -> Python connector).
• pytest tests/unit_tests/db_engine_specs/test_databricks.py tests/unit_tests/db_engine_specs/test_presto.py — 200 passed.
• pytest tests/unit_tests/db_engine_specs/test_init.py — 6 passed.
• pytest tests/integration_tests/db_engine_specs/databricks_tests.py::...::test_get_engine_spec — passed.
• pre-commit run --files ... — ruff, ruff-format, mypy, pylint, db-engine-spec metadata validation all pass.

TRADEOFFS

• Any Databricks connection currently hitting the ambiguous fallback (i.e. a
  stored URI whose driver suffix matches no registered spec exactly) will now be
  resolved as DatabricksPythonConnectorEngineSpec instead of
  DatabricksHiveEngineSpec.
• For the table_extra_metadata / get_create_view path this turns a hard crash
  into a working call.
• For other methods reached on the wrong-fallback path there may be a behavior
  change as well, but this is a strict improvement: the old Hive fallback was
  already broken for anything depending on a real Hive cursor (that's the crash),
  and the new fallback matches the connector that URI is actually
  resolvable/connectable with (databricks-sql-connector).
• Legitimate +pyhive / +pyodbc / +connector / +databricks-sql-python
  URIs are completely unaffected — exact driver match still wins before the
  fallback is ever consulted.
• The no-driver case get_engine_spec("databricks", None) (a bare
  databricks:// URI, exactly what the bug: Hive async kwarg unexpected keyword when connecting to Databricks Unity Catalog #24786 reporters used) is also affected:
  it skips the exact-match loop and hits the same fallback, so it now resolves to
  DatabricksPythonConnectorEngineSpec instead of DatabricksHiveEngineSpec.
  Same strict improvement — that URI connects via the Python connector.
• The ambiguous-fallback behavior itself is not Databricks-specific; it's the
  shared get_engine_spec mechanism (any backend with multiple specs and an
  unrecognized driver has the same "random first-defined spec" outcome). This PR
  only fixes it for Databricks via file ordering and deliberately does not
  change the shared mechanism (bigger blast radius). A general fix (e.g. an
  explicit fallback/priority marker) is a possible follow-up.

ADDITIONAL INFORMATION

• Has associated issue: relates to bug: Hive async kwarg unexpected keyword when connecting to Databricks Unity Catalog #24786
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #9b0bb9

Actionable Suggestions - 0

Additional Suggestions - 1

• superset/db_engine_specs/databricks.py - 1

  • Unguarded nested dict access · Line 909-909
    `get_default_catalog` indexes `get_extra_params(database)["engine_params"]["connect_args"]` with no guard. `get_extra_params` (base.py:2636) returns `json.loads(database.extra)` or `{}`, so a Native connection whose `extra` omits `engine_params.connect_args` raises KeyError instead of falling back to the SHOW CATALOGS logic. Use `.get()` chains or return None.

Review Details

• Files reviewed - 2 · Commit Range: f9b48ec..f9b48ec

  • superset/db_engine_specs/databricks.py
  • tests/unit_tests/db_engine_specs/test_databricks.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 77.33333% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (43fee87) to head (f9b48ec).

Files with missing lines	Patch %	Lines
superset/db_engine_specs/databricks.py	77.33%	15 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44449      +/-   ##
==========================================
- Coverage   80.65%   80.65%   -0.01%     
==========================================
  Files        2942     2942              
  Lines      175692   175692              
  Branches    40788    40788              
==========================================
- Hits       141705   141703       -2     
- Misses      31325    31327       +2     
  Partials     2662     2662              

Flag	Coverage Δ
hive	37.15% <56.00%> (ø)
mysql	56.38% <56.00%> (ø)
postgres	56.39% <56.00%> (-0.01%)	⬇️
presto	39.04% <56.00%> (ø)
python	84.96% <77.33%> (-0.01%)	⬇️
sqlite	56.10% <56.00%> (ø)
unit	76.77% <77.33%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

Suggestion: Unrecognized legacy URIs now select this spec, whose parser requires http_path, catalog, and schema; missing keys raise KeyError, causing database parameters to become empty.

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/db_engine_specs/databricks.py
**Line:** 596:600
**Comment:**
	*Api Mismatch: Unrecognized legacy URIs now select this spec, whose parser requires `http_path`, `catalog`, and `schema`; missing keys raise `KeyError`, causing database parameters to become empty.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[bito-code-review]

The flagged issue is correct. When an unrecognized legacy URI selects a spec that expects specific parameters (http_path, catalog, schema) but does not receive them, the parser raises a KeyError, leading to empty database parameters.

To resolve this, you should implement a validation check or provide default values within the validate_parameters or build_sqlalchemy_uri methods of the relevant engine spec to ensure these keys exist before the parser attempts to access them.

Example fix for superset/db_engine_specs/databricks.py:

def validate_parameters(properties: dict[str, Any], ...) -> list[dict[str, Any]]:
    # Ensure required keys exist to prevent KeyError
    required = {"http_path", "catalog", "schema"}
    for key in required:
        if key not in properties:
            properties[key] = ""  # Or appropriate default
    # ... existing validation logic

Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?

superset/db_engine_specs/databricks.py

required = {"http_path", "catalog", "schema"}
    for key in required:
        if key not in properties:
            properties[key] = ""  # Or appropriate default