sadpandajoe commented on code in PR #43695:
URL: https://github.com/apache/superset/pull/43695#discussion_r4450111060
##########
superset/db_engine_specs/databricks.py:
##########
@@ -1095,6 +1182,85 @@ def get_default_catalog(cls, database: Database) -> str:
return conn.execute(text("SELECT current_catalog()")).scalar()
+ @classmethod
+ def get_columns(
+ cls,
+ inspector: Inspector,
+ table: Table,
+ options: dict[str, Any] | None = None,
+ ) -> list[ResultSetColumnType]:
+ """
+ Get all columns from a given schema and table.
+
+ Resolves the server's actual default catalog dynamically if no catalog
+ is set on the connection/dialect, preventing reflection from generating
+ invalid queries like `DESCRIBE TABLE EXTENDED
`None`.`default`.`orders``
+ while leaving the connection handshake unforced.
+ """
+ dialect = getattr(inspector, "bind", None) and inspector.bind.dialect
+ if dialect is None and hasattr(inspector, "engine"):
+ dialect = inspector.engine.dialect
+
+ if dialect is not None and getattr(dialect, "catalog", None) is None:
+ engine = getattr(inspector, "bind", None) or getattr(
+ inspector, "engine", None
+ )
+ if engine is not None:
+ try:
+ with engine.connect() as conn:
+ catalogs = {
+ cat for (cat,) in conn.execute(text("SHOW
CATALOGS"))
+ }
+ if len(catalogs) == 1:
+ dialect.catalog = catalogs.pop()
+ else:
+ dialect.catalog = conn.execute(
+ text("SELECT current_catalog()")
+ ).scalar()
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Unable to resolve default catalog from server "
+ "during reflection",
+ exc_info=True,
+ )
+
+ return super().get_columns(inspector, table, options)
+
+ @classmethod
+ def get_table_names(
Review Comment:
This `get_table_names` override repeats the no-catalog resolution that
`get_columns` has, but none of the added tests call it, so deleting it leaves
every test green. With `dialect.catalog` unset and `SHOW CATALOGS` returning
`spark_catalog`, table listing would go back to reflecting against `None`.
Could a test mirror the `get_columns` one and assert `dialect.catalog ==
"spark_catalog"` before `inspector.get_table_names` is called, with `schema`
forwarded?
##########
superset/db_engine_specs/databricks.py:
##########
@@ -1011,6 +1020,49 @@ def build_sqlalchemy_uri( # type: ignore
query=query,
).render_as_string(hide_password=False)
+ @classmethod
+ def adjust_engine_params(
+ cls,
+ uri: URL,
+ connect_args: dict[str, Any],
+ catalog: str | None = None,
+ schema: str | None = None,
+ ) -> tuple[URL, dict[str, Any]]:
+ adjusted_connect_args = connect_args.copy()
+
+ effective_catalog = (
+ catalog
+ or adjusted_connect_args.get("catalog")
+ or uri.query.get("catalog")
+ or cls.default_catalog
+ )
+ effective_schema = (
+ schema
+ or adjusted_connect_args.get("schema")
+ or uri.query.get("schema")
Review Comment:
`server_default_catalog` falls back to `"main"` because
`DatabricksNativeEngineSpec` has no `default_catalog` attribute, so it isn't
the server's actual default. With a saved `/finance` URI and no configured
catalog, a workspace whose default is `spark_catalog`/`hive_metastore` (what
`get_default_catalog` returns) has its own default treated as a switch and
loses `finance`, while a workspace whose default is `prod` has a genuine switch
to `main` treated as the default and still sends `schema=finance`, which fails
if `finance` only exists in `prod`. Could the comparison use the resolved
default catalog, with a test whose default isn't `main`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
