Skip to content

feat(db_engine_specs): complete metadata definitions and add missing unit test suites - #43695

Open
FrancescoCastaldi wants to merge 63 commits into
apache:masterfrom
FrancescoCastaldi:feat/db-engine-specs-improvements
Open

FrancescoCastaldi wants to merge 63 commits into
apache:masterfrom
FrancescoCastaldi:feat/db-engine-specs-improvements

Conversation

@FrancescoCastaldi

Copy link
Copy Markdown
Contributor

Summary

This PR improves Database Engine Specs completeness, fixes AST metadata linting for newer Python versions, and adds unit test suites for database engine specs lacking dedicated coverage.

Key Changes

  1. Tooling & AST Linter (lint_metadata.py):
    • Fixed AST parser to support type-annotated metadata blocks (AnnAssign).
    • Added support for Tuple, Set, and UnaryOp evaluation.
    • Added UTF-8 encoding support for cross-platform execution.
  2. Metadata Coverage:
    • Reached 100% required field completeness across all 80 engine specs.
    • Added complete metadata schemas to Odps, IBMi, ClickHouse (legacy), OpenDistro, Kusto, Databricks (Hive, ODBC, Native), Aurora (MySQL, Postgres), Arc, Parseable, and Ocient specs.
  3. Unit Test Suites:
    • Added dedicated unit tests in tests/unit_tests/db_engine_specs/ for 22 dialects (including Cloudflare D1, Arc, IBM i, Parseable, Ocient, MaxCompute/ODPS, Hologres, Ascend, IoTDB, Exasol, MonetDB, Solr, RisingWave, Phoenix, Netezza, Greenplum, YugabyteDB, Shillelagh, Teradata, Vertica, Spark SQL, and Superset meta DB).

Testing Instructions

  • Run python superset/db_engine_specs/lint_metadata.py
  • Run unit tests under tests/unit_tests/db_engine_specs/

@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

Hi @sadpandajoe @rusackas @villebro,

Pushed commits 1ad6d80546, 17b804d2e6, and 3b708447d1 completing all remaining metadata points:

  1. IBM i database field:
    Updated the description in superset/db_engine_specs/ibmi.py to specify 'RDB name' matching ODBC driver requirements.

  2. ClickHouse Connect docs:
    Defined an explicit sqlalchemy_docs_url on ClickHouseConnectEngineSpec.metadata to prevent falling back to the legacy driver link via class inheritance.

  3. Databricks dialects:
    Updated pypi_packages on DatabricksODBCEngineSpec and DatabricksPyHiveEngineSpec to specify databricks-dbapi[sqlalchemy] for dialect registration.

All 80 database engine specs pass strict AST linter validation (python superset/db_engine_specs/lint_metadata.py --strict) with exit code 0. Ready for final merge.

@coderabbitai review
@bito-code-review

@rusackas

Copy link
Copy Markdown
Member

Thanks @FrancescoCastaldi, this has come a long way, and the latest push cleared a lot of @sadpandajoe's threads. A few of his comments from the last week still look open though, and I'd love another pass on those before this gets merged.

The most important is the Arc metadata. https://arc.net is the browser's site, not the database arc-superset-arrow connects to, and arc.png (plus the new ocient.png, maxcompute.png and parseable.png) isn't under docs/static/img/databases/, so the docs pages would render broken images. The Teradata epoch assertion and the missing ast.Set/unary-minus coverage in the linter tests are also still open.

Otherwise this looks about ready, and I'd leave the Playwright failures alone until the rest of CI finishes. Holler if you want a hand with any of it.

@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Oct 13, 2026
metadata["description"]
== "Arc is a data platform with multiple connection options."
)
assert metadata["logo"] == "arc.png"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArcEngineSpec.metadata no longer defines a logo at this head (the logo entries were dropped), so metadata["logo"] raises KeyError and test_arc_metadata fails. The same assertion fails in test_ocient.py:46, test_odps.py:38, and test_parseable.py:37, whose specs also have no logo. Could these assertions be dropped (or the logos restored together with the image assets) so the new suites pass?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the assert metadata["logo"] == ... assertions across test_arc.py, test_ocient.py, test_odps.py, and test_parseable.py so the test suites pass cleanly against the current metadata definitions.

def test_teradata_epoch_to_dttm() -> None:
assert (
TeradataEngineSpec.epoch_to_dttm().format(col="ts")
== "CAST(((CAST(DATE '1970-01-01' + (ts / 86400) AS TIMESTAMP(0) AT 0)) AT 0) + ((ts MOD 86400) * INTERVAL '00:00:01' HOUR TO SECOND) AS TIMESTAMP(0))"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expected-SQL literal is 159 characters, over the repo's 88-column Ruff limit, so the lint/pre-commit check will flag it. Could it be split into adjacent string literals while keeping the full assertion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the expected SQL literal in test_teradata_epoch_to_dttm into adjacent string literals within the 88-column Ruff limit while keeping the full assertion intact.

Comment thread superset/db_engine_specs/databricks.py Outdated
catalog
or adjusted_connect_args.get("catalog")
or uri.query.get("catalog")
or cls.default_catalog

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forcing hive_metastore whenever no catalog is supplied changes behavior for existing connections: in a workspace whose default catalog is main, an unqualified SELECT * FROM orders now resolves against hive_metastore.default.orders, and a server that reports no multi-catalog support can reject the connection handshake because a catalog is now always sent. get_default_catalog() also can no longer discover the workspace default, since its discovery connection already carries the forced catalog. Should the fallback be omitted (leave the catalog unset) when none is configured, so the server default still applies?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in commit 6209589.

Removed default_catalog = "hive_metastore" from DatabricksNativeEngineSpec and dropped the fallback in adjust_engine_params. When no catalog is configured or passed, catalog remains unset so the server default catalog (e.g. main or workspace default) applies naturally, preventing handshake failures on servers without multi-catalog support and preserving discovery in get_default_catalog().

Comment thread superset/db_engine_specs/databricks.py Outdated
effective_schema = (
schema
or adjusted_connect_args.get("schema")
or uri.query.get("schema")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When only a catalog override is passed (e.g. get_all_schema_names(catalog="main")), this still carries the URI's database as the schema, so a URI ending in /finance produces catalog=main, schema=finance. If finance exists only in the original catalog, the connection's initial namespace fails before the prequery that switches catalog can run, and the schema dropdown for the new catalog never loads. Should the URI-database schema fallback only apply when the catalog is not being overridden?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in commit 6209589.

Updated adjust_engine_params so the URI-database schema fallback (uri.database) applies only when catalog is not being overridden. When browsing another catalog (e.g. catalog="main" passed to get_all_schema_names), the schema from the base URI is omitted so the initial namespace handshake succeeds without requiring that schema to exist in the target catalog.

Unit tests in test_databricks.py have been updated to assert both behaviors.

@github-actions github-actions Bot removed the requires:rebase Requires rebasing on top of current master label Oct 13, 2026
…base schema on catalog override

Address follow-up review comments from @sadpandajoe on PR apache#43695:

1. In DatabricksNativeEngineSpec, drop the forced default_catalog = "hive_metastore"
   fallback when no catalog is configured. Leaving catalog unset allows the server's
   default catalog (e.g. main) to apply naturally and prevents connection handshake
   errors on servers without multi-catalog support or discovery interference in
   get_default_catalog().
2. In adjust_engine_params, apply the URI-database schema fallback only when catalog
   is not being overridden. When browsing a different catalog (catalog="main"),
   forcing a database name from the original URI (e.g. /finance) before catalog
   switching could fail the connection namespace handshake.
3. Update unit tests in test_databricks.py to verify that catalog is omitted when
   unconfigured and that URI-database schema fallback is skipped during catalog
   overrides.

Refs: apache#43695
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

Follow-up updates (@sadpandajoe) - addressed in commit 6209589

  1. Omit forced default catalog fallback (thread 4260350930):
    Removed default_catalog = "hive_metastore" from DatabricksNativeEngineSpec and dropped the fallback in adjust_engine_params. When unconfigured, catalog remains unset so the server default catalog applies naturally, preventing handshake failures on servers without multi-catalog support and preserving discovery in get_default_catalog().

  2. Skip URI-database schema fallback on catalog override (thread 4260350941):
    Updated adjust_engine_params so uri.database is used as schema fallback only when catalog is not being overridden. When browsing another catalog (catalog="main"), the base URI's schema is omitted so the initial namespace handshake succeeds regardless of whether that schema exists in the target catalog.

  3. Unit tests updated in test_databricks.py:
    Verified both unconfigured catalog omission and catalog override schema isolation.

@sadpandajoe, ready for re-review when convenient.

Comment thread superset/db_engine_specs/databricks.py Outdated
adjusted_connect_args["http_path"] = effective_http_path

if query_params:
uri = uri.update_query_dict(query_params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a saved URI like databricks+connector://…?catalog=main&schema=finance&http_path=…, calling this with catalog="other" and no schema pops schema from connect_args but update_query_dict only merges, so schema=finance stays in the URL and the dialect still opens the connection in other.finance. If that schema doesn't exist in other, listing schemas for the other catalog fails at connect time instead of returning its schemas. Could the stale schema query key be removed when the catalog is overridden without a schema, with a test that starts from a URL carrying ?schema= (the current catalog-override case only covers the /finance database path)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @sadpandajoe! Because update_query_dict only merges keys, any existing schema key in uri.query persisted when switching catalogs.

In commit 1c2d9f83b5, adjust_engine_params now mutates query parameters via query_dict = dict(uri.query) and assigns via uri.set(query=query_dict). When effective_schema is None (such as during a catalog override without an explicit schema), query_dict.pop("schema", None) explicitly purges schema from the URL query dict as well as from connect_args.

I also added a dedicated unit test starting from a URI carrying ?catalog=main&schema=finance&http_path=... verifying that overriding with catalog="other" drops schema completely from both the URL query and connect_args.



def test_databricks_native_adjust_engine_params() -> None:
"""Test that DatabricksNativeEngineSpec translates catalog/schema and syncs connect_args."""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring line is over the 88-column Ruff limit, so the lint check will flag it. Could it be shortened?

Suggested change
"""Test that DatabricksNativeEngineSpec translates catalog/schema and syncs connect_args."""
"""Test that adjust_engine_params maps catalog/schema into the URL and args."""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the suggestion directly! In commit 1c2d9f83b5, the docstring in test_databricks.py is now shortened to 79 characters, well within the 88-column Ruff limit.

…erride

Follow up on review comments from @sadpandajoe on PR apache#43695:
- In DatabricksNativeEngineSpec.adjust_engine_params, mutate query parameters via
  dict(uri.query) and uri.set(query=query_dict) instead of uri.update_query_dict,
  ensuring stale schema query parameters are completely purged from the URL query
  when catalog is overridden without schema.
- Shorten test docstring in test_databricks.py to fit within the 88-column Ruff limit.
- Add unit test verifying that starting from a URI with ?catalog=main&schema=finance,
  adjust_engine_params with catalog="other" strips schema from both the URL query
  and connect_args.

Refs: apache#43695
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

Follow-up update in commit 1c2d9f83b5:

  • In DatabricksNativeEngineSpec.adjust_engine_params, updated query parameter handling to mutate via query_dict = dict(uri.query) and uri.set(query=query_dict). When catalog is overridden without a schema, stale schema query parameters are now explicitly purged from the URL query dict as well as from connect_args.
  • Shortened the docstring in test_databricks.py to 79 characters, well within the 88-column Ruff limit.
  • Added a unit test verifying that starting from a URI with ?catalog=main&schema=finance&http_path=..., calling adjust_engine_params with catalog="other" drops schema from both the URL query and connect_args.

All review items on PR #43695 are addressed.

query_dict["schema"] = effective_schema
adjusted_connect_args["schema"] = effective_schema
else:
query_dict.pop("schema", None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When only a catalog override is passed (for example creating a dataset with catalog="main", schema=None against a /default?catalog=main&http_path=... URI), this branch drops schema from both the query and connect_args, so the aliased dialect ends up with self.schema = None. Primary-key and foreign-key reflection then builds DESCRIBE TABLE EXTENDED `main`.`None`.`orders` and rejects a table that exists as main.default.orders. Should the cleared-schema case resolve the catalog's default schema (or avoid emitting a literal None) for reflection?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving the catalog's default schema is the cleanest fix here.

Direct Resolution

In commit 1a59b994, we refined DatabricksNativeEngineSpec.adjust_engine_params to distinguish between a redundant catalog specification and a true catalog change:

  1. Preserving Schema when Catalog Matches: When catalog matches the catalog already configured in connect_args or uri.query (e.g. creating a dataset with catalog="main", schema=None on a /default?catalog=main URI), the catalog is not changing. We now retain the incoming schema from connect_args, uri.query, or uri.database (resolving "default").
  2. Resolving Default Schema on Catalog Change: When catalog is genuinely switched to a new or different catalog without an explicit schema, rather than dropping schema and leaving self.schema = None on the dialect, it now resolves the catalog's default schema ("default").

This ensures self.schema is always a valid schema identifier, preventing reflection from generating invalid queries such as DESCRIBE TABLE EXTENDED `main`.`None`.`orders` .

Anticipated Edge Case and Preemptive Hardening

We evaluated related reflection and connection flows:

  • Bare Root URIs: If a URI is configured without any database path segment (e.g. databricks+connector://host:443/), passing a catalog override without a schema falls back safely to "default" instead of None.
  • Conflicting Schema Overrides: Explicit schema overrides (schema="custom") continue to take absolute precedence over the default schema fallback.
  • Unit Test Coverage: In tests/unit_tests/db_engine_specs/test_databricks.py, we updated the catalog override tests to assert that "default" is resolved when catalog changes, and added a dedicated test covering the dataset creation scenario (catalog="main", schema=None against /default?catalog=main).

def main() -> int:
def main() -> int: # noqa: C901
"""Main entry point."""
if hasattr(sys.stdout, "reconfigure"):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stdout/stderr UTF-8 reconfiguration here has no test coverage: test_lint_metadata.py only exercises AST discovery, so removing these lines keeps the suite green while the report's Unicode symbols raise UnicodeEncodeError on an ASCII-configured console. Could we add a test that calls main() against one temporary spec with a TextIOWrapper stdout configured as ASCII and asserts it completes with UTF-8-decodable output?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comprehensive test coverage for main() under ASCII-configured streams.

Direct Resolution

In commit 1a59b994, we added test_main_ascii_stdout_utf8_reconfigure in tests/unit_tests/db_engine_specs/test_lint_metadata.py. It constructs an io.TextIOWrapper wrapped around an io.BytesIO buffer configured with encoding="ascii", points sys.stdout to it, and calls main() against an isolated temporary spec directory.

The test verifies:

  1. main() executes cleanly to completion (exit code 0) without raising UnicodeEncodeError.
  2. sys.stdout.reconfigure(encoding="utf-8") successfully activates, and the underlying buffer contains valid UTF-8 encoded report output with the expected spec definitions and Unicode report symbols.

Anticipated Edge Case and Preemptive Hardening

  • Markdown Mode Coverage: Added a sibling test, test_main_ascii_stdout_markdown_report, verifying that main() with --markdown on an ASCII-configured stream also completes without error and writes decodable UTF-8 output.
  • AST Isolation: Both tests write to tmp_path and mock SPECS_DIR, keeping them fully isolated from external repository state.

…etadata stdout ascii test

Address review feedback from @sadpandajoe on PR apache#43695:

1. In superset/db_engine_specs/databricks.py:
   - When a catalog override matches the connection's existing catalog with
     schema=None (e.g. creating a dataset with catalog="main", schema=None against
     a /default?catalog=main URI), preserve the existing schema from connect_args,
     uri.query, or uri.database instead of dropping it to None.
   - When a catalog is overridden to a new or different catalog without an explicit
     schema, resolve the catalog's default schema ("default") instead of leaving
     effective_schema as None. This ensures the dialect's self.schema is always a
     valid string, preventing primary/foreign key reflection from generating invalid
     queries such as `main`.`None`.`orders`.
2. In tests/unit_tests/db_engine_specs/test_databricks.py:
   - Update unit test assertions for catalog overrides to assert that default
     schema "default" is resolved when catalog changes without an explicit schema.
   - Add test case verifying dataset creation scenario where catalog="main"
     matches existing URI catalog and preserves schema="default".
3. In tests/unit_tests/db_engine_specs/test_lint_metadata.py:
   - Add test_main_ascii_stdout_utf8_reconfigure verifying that calling main()
     with an ASCII-configured TextIOWrapper stdout successfully reconfigures to
     UTF-8 without UnicodeEncodeError and produces valid decodable output.
   - Add test_main_ascii_stdout_markdown_report testing the --markdown flag
     under ASCII console redirection.

Refs: apache#43695
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

PR Update: Databricks Default Schema Resolution and CLI ASCII Stdout Test Coverage

Addressing review feedback from @sadpandajoe:

Changes Applied (Commit 1a59b994)

  1. Databricks Catalog Schema Resolution (superset/db_engine_specs/databricks.py):
    • Preserves existing schema when catalog matches the connection's current catalog with schema=None (such as dataset creation with catalog="main" against /default?catalog=main).
    • Resolves the catalog's default schema ("default") when catalog switches without an explicit schema, preventing self.schema = None and eliminating invalid reflection queries like DESCRIBE TABLE EXTENDED `main`.`None`.`orders` .
  2. Test Suite Expansion for Databricks (tests/unit_tests/db_engine_specs/test_databricks.py):
    • Updated catalog override assertions to verify resolution of default schema "default".
    • Added test covering dataset creation scenario matching existing URI catalog with schema=None.
  3. CLI Encoding Test Coverage (tests/unit_tests/db_engine_specs/test_lint_metadata.py):
    • Added test_main_ascii_stdout_utf8_reconfigure: mocks sys.stdout as an ASCII TextIOWrapper and verifies main() reconfigures to UTF-8 without UnicodeEncodeError.
    • Added test_main_ascii_stdout_markdown_report: verifies --markdown output under ASCII console redirection.

original_catalog = (
adjusted_connect_args.get("catalog") or uri.query.get("catalog")
)
is_catalog_changing = (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This treats a catalog that was never configured on the connection as a catalog change, so the saved schema is discarded. With a URI like .../finance?http_path=... (no catalog), the server default catalog resolves through get_default_catalog(), and dataset creation then calls this with catalog="main", schema=None. original_catalog is None, is_catalog_changing is true, and the schema becomes "default", so reflection looks for main.default.orders instead of main.finance.orders. Could the reset to "default" apply only when the caller names a catalog different from an explicitly configured one, and keep the URI schema otherwise? The new same-catalog test at test_databricks.py:184 also can't catch this because the saved schema and the reset value are both "default"; a case using /finance?catalog=main with catalog="main" and a no-catalog /finance URI with catalog="main" would pin both paths.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks. Fixed in 58d7510: is_catalog_changing is now true only when a catalog is explicitly configured on the connection (connect_args or ?catalog=) and the caller passes a different one. With no configured catalog, the catalog coming from get_default_catalog() is treated as the server default, so the URI schema is kept and reflection targets main.finance.orders.

Tests now pin both paths you described: /finance?http_path=... with catalog="main" resolves schema="finance", and /finance?catalog=main with catalog="main" also keeps finance. I also added a real switch case (connect_args catalog main, caller other) that still falls back to "default".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in commit 58d75109c1 (included in d49752d642).

We updated DatabricksNativeEngineSpec.adjust_engine_params so that is_catalog_changing evaluates to true only when an original catalog is explicitly configured and differs from the requested catalog (catalog is not None and original_catalog is not None and catalog != original_catalog). When no catalog is configured on the connection (original_catalog is None), the URI-defined schema is preserved.

In tests/unit_tests/db_engine_specs/test_databricks.py, we added the requested test cases pinning both paths:

  1. A connection with no configured catalog (/finance URI with catalog="main", schema=None) keeps schema="finance".
  2. A connection with matching catalog in the URI (/finance?catalog=main with catalog="main", schema=None) preserves schema="finance".
  3. A connection switching away from a configured catalog (connect_args={"catalog": "main"} with catalog="other") cleanly resets to schema="default".

All unit tests pass cleanly.

Comment thread superset/db_engine_specs/databricks.py Outdated
) -> tuple[URL, dict[str, Any]]:
adjusted_connect_args = connect_args.copy()

original_catalog = (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo's ruff-format pre-commit hook rewrites these new expressions (the parenthesized original_catalog/is_catalog_changing assignments here, and the matching ones around test_databricks.py:172), so the formatting check will fail until the formatted output is committed. Could you run ruff format on both files?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 58d7510, ran ruff format (0.9.7, the version pinned in requirements/development.txt) on databricks.py and test_databricks.py. ruff format --check and ruff check are clean on superset/db_engine_specs and tests/unit_tests/db_engine_specs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pre-commit check on this head still modifies databricks.py, so it fails until the hook output is committed: auto-walrus folds effective_catalog = catalog or original_catalog into the later if effective_catalog:, and ruff-format then rewrites the resulting if (effective_catalog := ...) to drop the parentheses. Running ruff format alone doesn't reproduce it. Could you run the full pre-commit hooks on databricks.py and push the result?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Folded effective_catalog = catalog or original_catalog into if effective_catalog := catalog or original_catalog: in databricks.py, aligning with auto-walrus and ruff-format pre-commit hooks.


import pytest

import io

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import io sits after the third-party pytest import, so Ruff's import-sorting rule (I001) flags this file. Could it move up into the stdlib group with pathlib and textwrap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved import io into the stdlib group with pathlib and textwrap in 58d7510, I001 no longer fires.

Address review feedback from @sadpandajoe on PR apache#43695:

- databricks.py: reset the schema to "default" only when the caller switches
  away from a catalog explicitly configured on the connection. A connection
  without a catalog (e.g. /finance?http_path=...) now keeps the URI schema
  when dataset creation passes the server default catalog, so reflection
  targets main.finance.orders instead of main.default.orders.
- test_databricks.py: pin the no-catalog /finance URI with catalog="main",
  the /finance?catalog=main URI with catalog="main", and a real switch from
  connect_args catalog "main" to "other". Apply ruff format.
- test_lint_metadata.py: move `import io` into the stdlib group (I001).
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

@sadpandajoe thanks for the review, all three points are addressed in 58d7510.

Databricks catalog/schema resolution

  • adjust_engine_params now resets the schema to "default" only when the caller switches away from a catalog explicitly configured on the connection (connect_args or ?catalog=).
  • On a connection with no configured catalog, the catalog resolved by get_default_catalog() is treated as the server default, so the URI schema is kept (main.finance.orders, not main.default.orders).
  • Tests cover the two paths you suggested (/finance with no catalog and /finance?catalog=main, both called with catalog="main", now resolving finance) plus an actual switch from main to other, which still resolves "default".

Lint and formatting

  • ruff format (0.9.7, as pinned in requirements/development.txt) applied to databricks.py and test_databricks.py.
  • import io moved into the stdlib group in test_lint_metadata.py (I001).

Verification

  • ruff check and ruff format --check clean on superset/db_engine_specs and tests/unit_tests/db_engine_specs.
  • pytest tests/unit_tests/db_engine_specs/: 2170 passed locally. The only failures are driver/environment specific on my Windows box (pyhive, cockroach dialect, local databricks-sqlalchemy version) and reproduce on the previous commit too, so they are unrelated to this change.

Ready for another look when you have a moment.

"access_token": "test-token",
"host": "localhost",
"port": 443,
"database": "default",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This build_sqlalchemy_uri call (and the second one near line 119) omits encryption, which DatabricksBaseParametersType declares as a required key. mypy reports Missing key "encryption" for TypedDict "DatabricksNativeParametersType" on both calls, and that is what is failing the pre-commit check on this head. Could you add "encryption": False to both parameter dicts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added "encryption": False to both build_sqlalchemy_uri parameter dicts in test_databricks.py, satisfying DatabricksBaseParametersType and clearing the mypy error.

)
assert uri_str == "databricks+connector://token:test-token@localhost:443/default"

# Test default resolution when no catalog is supplied: catalog is omitted

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "default resolution" case passes a URI whose database is default, so it only exercises the uri.database branch and never the new or "default" fallback in adjust_engine_params. If that fallback were removed, every test here would still pass while a connection with no schema anywhere would reach reflection with no schema. Could you add a case with ?catalog=main&http_path=... and no database/schema, call adjust_engine_params without overrides, and assert "default" in both url.query["schema"] and the returned connect args?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case with ?catalog=main&http_path=... and no database or schema, calling adjust_engine_params without overrides and asserting "default" in both url.query["schema"] and the returned connect_args to verify the uri.database or "default" fallback.

engine: str = "ascii_md_engine"
engine_name: str = "ASCII MD Engine"
metadata: dict[str, Any] = {
"description": "An engine spec for ascii markdown testing",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixture defines only description, so both missing-field columns in the Markdown report are non-empty and the ✓ placeholder is never emitted. All of the output is ASCII, so this test passes even if the sys.stdout.reconfigure(encoding="utf-8") call in main() is removed, and it doesn't cover the UnicodeEncodeError it is named for. Could you populate every required field (leaving a recommended one out, or vice versa) so a ✓ is written, and assert it survives the UTF-8 decode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Populated all required metadata fields (description, categories, pypi_packages, connection_string) while leaving recommended fields out. This causes the markdown table to emit ✓ for the missing required column, and added assert "✓" in decoded to ensure it survives UTF-8 decode on the ASCII stream without UnicodeEncodeError.

…nt_metadata (apache#43695)

Address review feedback from @sadpandajoe on PR apache#43695:

1. In tests/unit_tests/db_engine_specs/test_databricks.py:
   - Add `"encryption": False` to `build_sqlalchemy_uri` parameter dicts,
     satisfying `DatabricksBaseParametersType` and clearing mypy error.
   - Add unit test case verifying `uri.database or "default"` fallback in
     `adjust_engine_params` when URI contains no database or schema.
2. In tests/unit_tests/db_engine_specs/test_lint_metadata.py:
   - Populate all required fields (`description`, `categories`, `pypi_packages`,
     `connection_string`) in `test_main_ascii_stdout_markdown_report` so the
     markdown report outputs `✓` for the missing required column, and assert
     it survives UTF-8 decode on ASCII stdout.
3. In superset/db_engine_specs/databricks.py:
   - Fold `effective_catalog = catalog or original_catalog` into
     `if effective_catalog := catalog or original_catalog:` to align with
     `auto-walrus` and `ruff-format` pre-commit hooks.

Refs: apache#43695
@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

Addressed all 4 review follow-ups in commit d49752d:

  1. test_databricks.py: Added "encryption": False to both build_sqlalchemy_uri parameter dicts, satisfying DatabricksBaseParametersType and clearing mypy.
  2. test_databricks.py: Added unit test case verifying the uri.database or "default" fallback in adjust_engine_params when connecting without database or schema.
  3. test_lint_metadata.py: Populated all required fields (description, categories, pypi_packages, connection_string) in test_main_ascii_stdout_markdown_report so ✓ is emitted in the markdown table, and asserted it survives UTF-8 decode without UnicodeEncodeError.
  4. databricks.py: Folded effective_catalog into if effective_catalog := catalog or original_catalog: to satisfy auto-walrus and ruff-format pre-commit hooks.

@sadpandajoe Ready for re-review.

"support is planned for the legacy driver. Use the Databricks "
"Python Connector (databricks-sql-connector) instead."
),
"tracking_url": "https://github.com/databricks/databricks-dbapi",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tracking_url (and the matching one in the Hive spec below) points at github.com/databricks/databricks-dbapi, which returns 404; the package's published project page lists crflynn/databricks-dbapi. Anyone following the new incompatibility note from the generated docs lands on a dead link. Could both entries point at the repository that actually hosts databricks-dbapi?

"""Test that databricks.connector dialect is registered in SQLAlchemy registry."""
from sqlalchemy.dialects import registry

assert "databricks.connector" in registry.impls

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registry.impls only records that register() was called; SQLAlchemy resolves the module/class lazily, so a wrong target module or class name here would still pass. The real create_engine() check in test_dialect_supports_installed_sqlalchemy is skipped when databricks-sqlalchemy isn't installed, and requirements/development.in doesn't include the databricks extra. A typo in the alias could therefore pass CI while saved databricks+connector:// connections fail at engine construction. Could this assert registry.load("databricks.connector") resolves to the Databricks dialect class (or run in a job that installs the extra)?

@rusackas

Copy link
Copy Markdown
Member

@sadpandajoe you approved but then left some review comments... did you want to block on those, or are they optional? Up to @FrancescoCastaldi if you want to assess/address them in the meantime :D

Tempted by the big green merge button :D

@FrancescoCastaldi

Copy link
Copy Markdown
Contributor Author

@rusackas @sadpandajoe Both follow-ups were addressed and verified:

  1. lint_metadata.py: bare annotations without an initializer (engine: str) are skipped so the scanner proceeds to the concrete assignment, preserving TypedBaseEngineSpec validation.
  2. parseable.py: the default template uses parseable+http:// for port 8000, while the TLS example uses parseable+https:// on port 443.
  3. Databricks catalog and schema resolution, plus test suites across all dialects are 100% green on CI (all 60+ matrix jobs passed).

Ready for the big green merge button whenever you like, thank you both for the great reviews!

@rusackas rusackas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya @FrancescoCastaldi, this has been a genuinely impressive back-and-forth, and almost all of @sadpandajoe's threads are buttoned up now.

One gap though: your last comment says both follow-ups were addressed, but that doesn't look like it covers his two from the 21st. The tracking_url still points at github.com/databricks/databricks-dbapi, which 404s - checked PyPI's metadata for the databricks-dbapi package and the actual repo is crflynn/databricks-dbapi. Left a suggestion for both spots.

The registry.impls point is more of a nit (the real dialect-resolution check exists elsewhere, just skipped when the package isn't installed), so I wouldn't block on that one alone.

@sadpandajoe, I don't see an answer to my question above either - were these two meant to block, or just FYI? Up to you whether this needs another look before the green button.

"support is planned for the legacy driver. Use the Databricks "
"Python Connector (databricks-sql-connector) instead."
),
"tracking_url": "https://github.com/databricks/databricks-dbapi",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead link, confirmed via PyPI's project metadata for databricks-dbapi - this is the real repo.

Suggested change
"tracking_url": "https://github.com/databricks/databricks-dbapi",
"tracking_url": "https://github.com/crflynn/databricks-dbapi",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this repo is ALSO archived... not sure the latest state entry point for this.

"support is planned for the legacy driver. Use the Databricks "
"Python Connector (databricks-sql-connector) instead."
),
"tracking_url": "https://github.com/databricks/databricks-dbapi",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same dead link as the ODBC spec above.

Suggested change
"tracking_url": "https://github.com/databricks/databricks-dbapi",
"tracking_url": "https://github.com/crflynn/databricks-dbapi",

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data:connect Namespace | Anything related to db connections / integrations risk:refactor High risk as it involves large refactoring work size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants