FrancescoCastaldi commented on code in PR #43695:
URL: https://github.com/apache/superset/pull/43695#discussion_r4272859995
##########
superset/db_engine_specs/databricks.py:
##########
@@ -1011,6 +1019,56 @@ 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()
+
+ is_catalog_overridden = catalog is not None
+ effective_catalog = (
+ catalog
+ or adjusted_connect_args.get("catalog")
+ or uri.query.get("catalog")
+ )
+ effective_schema = (
+ schema
+ or (adjusted_connect_args.get("schema") if not
is_catalog_overridden else None)
+ or (uri.query.get("schema") if not is_catalog_overridden else None)
+ or (uri.database if uri.database and not is_catalog_overridden
else None)
+ )
+ effective_http_path = (
+ adjusted_connect_args.get("http_path")
+ or uri.query.get("http_path")
+ )
+
+ query_dict = dict(uri.query)
+ if effective_catalog:
+ query_dict["catalog"] = effective_catalog
+ adjusted_connect_args["catalog"] = effective_catalog
+ else:
+ query_dict.pop("catalog", None)
+ adjusted_connect_args.pop("catalog", None)
+
+ if effective_schema:
+ query_dict["schema"] = effective_schema
+ adjusted_connect_args["schema"] = effective_schema
+ else:
+ query_dict.pop("schema", None)
Review Comment:
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`).
##########
superset/db_engine_specs/lint_metadata.py:
##########
@@ -605,11 +643,18 @@ def generate_markdown_report(reports:
list[MetadataReport]) -> str:
"",
]
)
-
return "\n".join(lines)
-def main() -> int:
+def main() -> int: # noqa: C901
+ """Main entry point."""
+ if hasattr(sys.stdout, "reconfigure"):
Review Comment:
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.
--
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]
