mikebridge commented on code in PR #44283: URL: https://github.com/apache/superset/pull/44283#discussion_r4020298636
########## superset/migrations/versions/2026-09-15_00-00_f7b3a9c14e02_migrate_clickhouse_to_clickhousedb.py: ########## @@ -0,0 +1,265 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""migrate legacy ClickHouse connections onto the clickhouse-connect driver + +Superset ships two ClickHouse engine specs. ``ClickHouseEngineSpec`` (backend +``clickhouse``, the ``clickhouse-sqlalchemy`` driver) is the legacy spec and has +no dynamic parameters schema, so its connections still render the free-form +SQLAlchemy URI editor. ``ClickHouseConnectEngineSpec`` (backend ``clickhousedb``, +default driver ``connect``, the ``clickhouse-connect`` driver) defines a +``parameters_schema`` and therefore renders the new dynamic connection form. + +This migration rewrites the ``sqlalchemy_uri`` of connections stored on the +legacy ``clickhouse`` backend so they resolve to ``ClickHouseConnectEngineSpec`` +and expose the new form. The backend name is what selects the engine spec +(``Database.db_engine_spec`` reads ``url.get_backend_name()``), so the scheme is +rewritten from ``clickhouse[+driver]://`` to ``clickhousedb+connect://``. + +Scheme and port mapping +----------------------- + +* ``clickhouse://`` and ``clickhouse+http://`` -- the ``clickhouse-sqlalchemy`` + HTTP dialect, default port 8123. ``clickhouse-connect`` also speaks HTTP on + 8123, so only the scheme changes; host, port, credentials, database and query + string are preserved byte-for-byte. +* ``clickhouse+native://`` and ``clickhouse+asynch://`` -- the native TCP + protocol, default port 9000. ``clickhouse-connect`` cannot speak the native + protocol; it is HTTP-only. A native connection therefore also needs its port + moved to the HTTP interface. When the port is the native default (9000, or + absent -- which implies 9000) it is rewritten to the HTTP default (8123). + A *non-default* native port cannot be mapped to an HTTP port without knowing + the server's configuration, so those rows are left on the legacy driver for a + human to migrate rather than pointed at a guessed port. Operators should + confirm the HTTP interface is enabled on the rewritten native connections. + +TLS: ``clickhouse-sqlalchemy`` enables TLS with ``?protocol=https``, but +``clickhouse-connect`` ignores ``protocol`` and reads ``secure`` instead, so a +preserved ``protocol=https`` would silently downgrade an encrypted connection to +plaintext HTTP. ``protocol=https`` is therefore rewritten to ``secure=true`` and +the meaningless ``protocol`` parameter is dropped; ``secure`` values that a +connection already carries are left untouched. + +Password handling: the credential lives either inline in the URI or in +``encrypted_extra``; both are preserved because the URI is edited in place (only +the scheme, the TLS parameter, and for native rows the port, are touched) and +``encrypted_extra`` is never read. + +Only rows whose backend is exactly ``clickhouse`` are rewritten; rows already on +``clickhousedb`` are skipped, which also makes ``upgrade`` idempotent. A row +whose rewritten URI would exceed the ``dbs.sqlalchemy_uri`` column length is +skipped with a warning rather than truncated. + +Downgrade +--------- + +``downgrade`` is intentionally a no-op. After ``upgrade`` a migrated row is +byte-for-byte indistinguishable from a connection an operator created directly on +the ``clickhousedb`` key (both are produced by ``build_sqlalchemy_uri`` as +``clickhousedb+connect://...``), so a blind reversal would also revert genuine +``clickhousedb`` connections onto the legacy driver and break them. The migrated +connections keep working after a code rollback regardless, because +``ClickHouseConnectEngineSpec`` predates this migration -- no data restoration is +required for them to remain functional -- so the safe choice is to leave the +rewritten URIs in place. + +The revision tree had two open heads when this was authored -- ``88a01c781622`` +(``index_ab_user_lower_username``) and ``c7f53d184ea2`` +(``coordinate_purge_audit_pruning``), which forked at ``7e2c9a4f1b83``. This +revision merges them so the tree keeps a single head; the data rewrite itself is +independent of both branches. + +Revision ID: f7b3a9c14e02 +Revises: 88a01c781622, c7f53d184ea2 +Create Date: 2026-09-15 00:00:00.000000 + +""" + +import logging + +from alembic import op +from sqlalchemy import Column, Integer, String +from sqlalchemy.orm import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update + +logger = logging.getLogger("alembic") + +# revision identifiers, used by Alembic. +revision = "f7b3a9c14e02" +down_revision = ("88a01c781622", "c7f53d184ea2") Review Comment: #44288 landed (`4c27cbd007`) and consumed both of these parents into `e2f3a1b9c640`, so master is single-headed again. As written this migration takes the same two parents, which makes it a second childless head — re-forking the graph the moment it merges. ```suggestion down_revision = "e2f3a1b9c640" ``` Worth knowing for anyone tracking the earlier CI breakage: `enforce-single-migration-head` will catch this *here* because this PR touches `superset/migrations/`, but the two-head state is invisible to every PR that does not — it surfaces instead as `docker-build (dev)` failing on `flask db upgrade`, which cascades into the whole integration matrix. -- 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]
