bmuraykhi opened a new pull request, #45961: URL: https://github.com/apache/superset/pull/45961
### SUMMARY **Sync columns from source** fails with `Unserializable object ... of type <class 'sqlalchemy.sql.elements.TextClause'>` on ClickHouse tables with ALIAS, MATERIALIZED, DEFAULT or TTL columns. Fixes #45960. **Why:** since clickhouse-connect 1.1.0, the dialect's `get_columns` returns those expressions as SQLAlchemy `text()` clauses (`clickhouse_alias`, `clickhouse_materialized`, `server_default`, `clickhouse_ttl`). `convert_inspector_columns` copies every inspector key into the result, and `external_metadata_by_name` sends it through `json_response`, which can't encode a clause. The documented Docker install pulls the latest driver, and since #40944 7.0 requires 1.1.1 or later, so every ClickHouse install on 7.0 hits it. **Fix:** `convert_inspector_columns` turns any SQLAlchemy `ClauseElement` value into its SQL string, so each key keeps a readable value (`"clickhouse_alias": "price * qty"`). Everything else passes through unchanged, including the `TypeEngine` in `type` that `get_physical_table_metadata` converts afterwards. **Why here instead of the ClickHouse engine spec:** this function is where reflected columns become the `ResultSetColumnType` dicts Superset serializes, and the extra keys are whatever each dialect chooses to return. Handling clauses here also covers any other dialect that returns one, and keeping the keys (as strings) instead of dropping them means nothing that reads these dicts loses data. **Driver side:** the clauses came with ClickHouse/clickhouse-connect#758's Alembic support in 1.1.0, and SQLAlchemy's `ReflectedColumn` expects a string there, so I reported it as ClickHouse/clickhouse-connect#1141. This fix is needed either way: 7.0 requires a driver that already returns them. **Scope:** the two endpoints that return a physical table's reflected columns as JSON, `/datasource/external_metadata_by_name/` (used by the editor) and `/datasource/external_metadata/<type>/<id>/`, both go through this function and are both fixed. Creating a dataset, `PUT /api/v1/dataset/<id>/refresh` and SQL Lab's table schema already worked, since they read only the fields they need. Virtual datasets take a different path and were never affected. **Driver versions:** the fix only converts clause values, so drivers that return none are unaffected. Checked against ClickHouse 26.10 on a table with ALIAS, MATERIALIZED, DEFAULT and TTL columns: | clickhouse-connect | Superset | Before | After | |---|---|---|---| | 0.13.0, 0.15.1, 1.0.0 | 6.1.0 (SQLAlchemy 1.4) | works, no clauses returned | unchanged | | 1.1.0, 1.11.0 | 6.1.0 (SQLAlchemy 1.4) | `Unserializable object` | works, expressions as strings | | 1.7.2, 1.11.0 | `master` (SQLAlchemy 2.0) | `Unserializable object` | works, expressions as strings | Drivers before 1.7 don't run on SQLAlchemy 2.0, so they don't apply to `master` or 7.0. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF **Before:** on 7.0.0rc2, after `ALTER TABLE t ADD COLUMN discount Float64 ALIAS price * 0.1`, **Sync columns from source** shows the error and adds nothing (Columns 4): <img width="3504" height="3960" alt="before" src="https://github.com/user-attachments/assets/5b42992d-8bd7-4a26-8b19-5969899c4214"; /> **After:** the same click adds `discount` (Columns 5). rc2's toast still says "virtual dataset"; master already says "dataset": <img width="3504" height="3960" alt="after" src="https://github.com/user-attachments/assets/302abe75-dd95-41e3-a8c8-04471ce8e9c6"; /> The endpoint for table `t`: ``` before: 500 {"error": "Unserializable object price * qty of type <class 'sqlalchemy.sql.elements.TextClause'>"} after: 200 [..., {"column_name": "total", "name": "total", "type": "FLOAT64", "nullable": false, "autoincrement": false, "comment": null, "clickhouse_codec": null, "clickhouse_ttl": null, "clickhouse_alias": "price * qty", "type_generic": 0, "is_dttm": false}] ``` ### TESTING INSTRUCTIONS Manual (ClickHouse with the latest clickhouse-connect, 1.11.0; any 1.1.0 or later shows it): 1. `CREATE TABLE t (ts DateTime, price Float64, qty UInt32, total Float64 ALIAS price * qty) ENGINE = MergeTree ORDER BY ts` 2. Add `t` as a dataset, then run `ALTER TABLE t ADD COLUMN discount Float64 ALIAS price * 0.1`. 3. Edit the dataset, open the Columns tab and click **Sync columns from source**. Before: an error toast and no new column. After: `discount` is added. Unit tests: ```bash pytest tests/unit_tests/db_engine_specs/test_base.py -k convert_inspector_columns ``` Against a real ClickHouse (needs Docker; CI runs it in `testcontainers.yml`): ```bash pytest -m testcontainers tests/testcontainers/db_engine_specs/test_clickhouse.py ``` The unit test covers every key clickhouse-connect sets as a clause, plus values that must pass through unchanged (`TypeEngine`, bools, `None`). The testcontainers test runs `get_physical_table_metadata` on a real table with ALIAS, MATERIALIZED and DEFAULT columns and serializes the result like `json_response` does. Both fail on `master` and pass here, and the patch is fully covered. ### ADDITIONAL INFORMATION - [x] Has associated issue: Fixes #45960 - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
