Copilot commented on code in PR #45961:
URL: https://github.com/apache/superset/pull/45961#discussion_r4458726569
##########
tests/testcontainers/db_engine_specs/test_clickhouse.py:
##########
@@ -137,6 +137,47 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
assert isinstance(spec.sqla_type, Integer)
+def test_physical_table_metadata_serializes_expression_columns(
+ engine: Engine,
+) -> None:
+ from tests.integration_tests.test_app import app
+
+ with app.app_context():
+ from superset.connectors.sqla.utils import get_physical_table_metadata
+ from superset.db_engine_specs.clickhouse import
ClickHouseConnectEngineSpec
+ from superset.utils import json
+
+ with engine.begin() as conn:
+ conn.execute(
+ text(
+ "CREATE TABLE pilot_expressions ("
+ "ts DateTime, price Float64, qty UInt32, "
+ "total Float64 ALIAS price * qty, "
+ "day Date MATERIALIZED toDate(ts), "
+ "note String DEFAULT 'x'"
+ ") ENGINE = MergeTree ORDER BY ts"
+ )
+ )
+
+ database = MagicMock()
+ database.db_engine_spec = ClickHouseConnectEngineSpec
+ database.get_dialect.return_value = engine.dialect
+ database.get_extra.return_value = {}
+ database.get_columns.side_effect = (
+ lambda table: ClickHouseConnectEngineSpec.get_columns(inspect(engine),
table)
+ )
+
+ columns = get_physical_table_metadata(
+ database, Table("pilot_expressions"), normalize_columns=False
+ )
+
+ by_name = {col["column_name"]: dict(col) for col in columns}
+ assert by_name["total"]["clickhouse_alias"] == "price * qty"
+ assert by_name["day"]["clickhouse_materialized"] == "toDate(ts)"
+ assert by_name["note"]["server_default"] == "'x'"
+ json.dumps(columns, default=json.json_int_dttm_ser, ignore_nan=True)
+
+
Review Comment:
The `app.app_context()` is entered only for the imports, but the call to
`get_physical_table_metadata(...)` happens after the context manager exits. If
`get_physical_table_metadata` (or any of its dependencies) relies on Flask
`current_app`/app config, this test can fail or become order-dependent. Keep
the app context active for the full section that calls
`get_physical_table_metadata` and performs `json.dumps(...)`.
##########
tests/testcontainers/db_engine_specs/test_clickhouse.py:
##########
@@ -137,6 +137,47 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
assert isinstance(spec.sqla_type, Integer)
+def test_physical_table_metadata_serializes_expression_columns(
+ engine: Engine,
+) -> None:
+ from tests.integration_tests.test_app import app
+
+ with app.app_context():
+ from superset.connectors.sqla.utils import get_physical_table_metadata
+ from superset.db_engine_specs.clickhouse import
ClickHouseConnectEngineSpec
+ from superset.utils import json
+
+ with engine.begin() as conn:
+ conn.execute(
+ text(
+ "CREATE TABLE pilot_expressions ("
+ "ts DateTime, price Float64, qty UInt32, "
+ "total Float64 ALIAS price * qty, "
+ "day Date MATERIALIZED toDate(ts), "
+ "note String DEFAULT 'x'"
+ ") ENGINE = MergeTree ORDER BY ts"
+ )
+ )
Review Comment:
This test creates a permanent table name without cleanup. If the `engine`
fixture is reused across tests (or a previous run left state behind), `CREATE
TABLE` can fail with “table already exists”, making the suite flaky. Consider
using `DROP TABLE IF EXISTS pilot_expressions` before creating, or `CREATE
TABLE IF NOT EXISTS`, and/or dropping the table in teardown.
--
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]
