aminghadersohi commented on code in PR #46055:
URL: https://github.com/apache/superset/pull/46055#discussion_r4494112567
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -178,6 +178,10 @@ def normalize_custom_sql_metric(cls, expression: str) ->
str:
supports_catalog = supports_dynamic_catalog =
supports_cross_catalog_queries = True
supports_grouping_sets = True
+ # Unquoted identifiers are case-insensitive: `information_schema.databases`
+ # lists `SALES`, while the URI (and so the default catalog) usually says
`sales`.
+ case_insensitive_catalogs = True
Review Comment:
I fixed the remaining connection-level catalog gap in
68ccae29eb45647fbec11dc20ac15107157f1138. `adjust_engine_params` quotes
physical catalog arguments, including embedded double quotes, before putting
them in the connection URI. `get_default_catalog` still folds unquoted URI
identifiers to uppercase and preserves quoted identifiers as exact physical
names.
Regression: `test_connection_catalog_preserves_physical_identity` exercises
`Database._get_sqla_engine` with quoted/encoded URI defaults and
mixed/lower/uppercase selected catalogs, with and without a schema. Before: 20
failed, including `assert 'Sales_Db' == '"Sales_Db"'`. After: all 20 pass. The
Docker run covering Snowflake, permission isolation, SQL parsing, database
engines and MCP workers passed 2,319 tests without the optional Snowflake
driver. A separate offline check with the real Snowflake SQLAlchemy dialect
passed 10/10 connect-argument cases; no warehouse connection was opened.
Pre-commit passed on the PR's changed files.
The old CI logs also identify a PR test-fixture problem: 14 SQL-permission
cases in `test_quoted_catalog_grant_is_isolated`,
`test_unquoted_sql_catalog_uses_uppercase_grant` and
`test_quoted_catalog_grant_does_not_cover_unquoted_sql` raised
`NoSuchModuleError: Can't load plugin: sqlalchemy.dialects:snowflake` while
constructing Jinja context. The fixture now supplies a generic SQLAlchemy
dialect; SQLGlot parsing and real permission checks remain exercised, without
adding an optional driver requirement.
Siblings checked:
- superset/db_engine_specs/snowflake.py:423 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/db_engine_specs/snowflake.py:472 — not affected: default-catalog
resolution returns the correct physical name; the connection boundary quotes it.
- superset/db_engine_specs/snowflake.py:484 — not affected: catalog listings
preserve exact physical names.
- superset/db_engine_specs/snowflake.py:330 — not affected: OAuth
impersonation changes authentication fields, not the catalog.
- superset/db_engine_specs/snowflake.py:581 — not affected: URI creation and
parameter round-tripping retain user-entered identifier syntax rather than
consuming physical catalog arguments.
- superset/models/core.py:731 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/models/core.py:860 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/models/core.py:1394 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/sql_lab.py:597 — fixed in 68ccae29eb45647fbec11dc20ac15107157f1138
- superset/sql_lab.py:911 — fixed in 68ccae29eb45647fbec11dc20ac15107157f1138
- superset/sqllab/sqllab_execution_context.py:132 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/sqllab/api.py:356 — not affected: serializes query metadata, not
a connection URI.
- superset/sqllab/schemas.py:47 — not affected: request/response field
declarations do not construct connections.
- superset/sqllab/permalink/schemas.py:37 — not affected: stores catalog
metadata without rewriting it.
- superset/sqllab/permalink/types.py:21 — not affected: type declaration
only.
- superset/sqllab/utils.py:43 — not affected: metadata field allowlist only.
- superset/security/manager.py:2551 — not affected: Snowflake permission
comparisons remain exact; catalog filtering and authorization do not construct
connection URIs.
- superset/commands/database/tables.py:179 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/commands/database/validate_sql.py:81 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/commands/database/sync_permissions.py:219 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/commands/database/utils.py:291 — fixed in
68ccae29eb45647fbec11dc20ac15107157f1138
- superset/commands/database/update.py:98 — not affected: retains physical
catalog metadata; connection construction uses the shared engine hook.
- superset/commands/database/create.py:95 — not affected: delegates
permission sync rather than constructing catalog URIs.
- superset/commands/database/importers/v1/utils.py:101 — not affected:
delegates permission sync rather than constructing catalog URIs.
- superset/commands/database/uploaders/base.py:172 — not affected: uses the
physical default for stored dataset identity; uploads use the configured URI
through the shared engine hook.
- superset/commands/database/test_connection.py:174 — not affected: connects
with the configured URI, without a physical catalog override.
- superset/commands/database/validate.py:206 — not affected: connects with
the configured URI, without a physical catalog override.
Remaining: new-head CI is pending. The old run also failed
`test_second_request_finishes_during_sleeping_query[sql_lab]` at `await slow`
with `MCP tool timed out after 2 seconds`; its worker/test code is unchanged
from master, whose base build passed, and the local pinned-dependency run
passed. I classify that separately as an unrelated timing-sensitive failure;
its precise timeout cause is unknown. I have not changed that test or claimed
CI is green. No live Snowflake acceptance test was run.
--
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]
