codeant-ai-for-open-source[bot] commented on code in PR #41176: URL: https://github.com/apache/superset/pull/41176#discussion_r3537953391
########## superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py: ########## @@ -0,0 +1,566 @@ +# 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. +"""add_versioning_tables + +Creates the full schema backing entity versioning in a single +migration: + +1. ``version_transaction`` — audit log keyed by Continuum's per-flush + transaction id (plus a Postgres-specific id sequence). +2. **Parent shadow tables** mirroring each versioned entity's columns: + ``dashboards_version`` / ``slices_version`` / ``tables_version``. +3. ``version_changes`` — field-level diff log keyed to a + ``(transaction, entity)`` pair; each row describes one atomic change + (one field or one child-collection element) that occurred during a + save. +4. **Child shadow tables** for the collections Continuum auto-registers + when ``__versioned__`` is applied to ``TableColumn`` / ``SqlMetric`` + and the ``slices`` exclude is removed from + ``Dashboard.__versioned__``: ``table_columns_version`` / + ``sql_metrics_version`` / ``dashboard_slices_version``. + +All shadow tables follow the validity-strategy shape (mirrored columns ++ ``transaction_id`` / ``end_transaction_id`` / ``operation_type`` +bookkeeping with FKs to ``version_transaction.id``). The current +version row has ``end_transaction_id = NULL``. + +This migration replaces three iterative migrations from the spike phase +(``56cd24c07170``, ``e1f3c5a7b9d0``, ``f7a2b3c4d5e6``) that captured the +same schema in three steps as the feature was developed. Compacting +gives downstream operators one migration to apply / reverse and one +review surface. The ``revision`` hash is reused from the original first +migration so anyone still tracking the chain by that hash lands on the +same logical change set. + +Generated by hand because the current Continuum + Alembic-autogenerate +interaction trips on the renamed ``transaction`` -> ``version_transaction`` +table key (``KeyError`` lookups in ``table_key_to_table``). Column +inventories were sourced from the live model ``__table__`` definitions +and ``version_class(...).__table__`` / Continuum association metadata. + +Primary key choice. Both ``version_transaction.id`` and +``version_changes.id`` are ``BigInteger`` autoincrement — a deliberate +carveout from the project's UUID-PK convention for new models (see +``CLAUDE.md`` §"UUID Migration"). ``version_transaction`` is keyed +externally by SQLAlchemy-Continuum via +``nextval('version_transaction_id_seq')`` on every INSERT; matching +that contract is required for ``versioning_manager`` to function. +``version_changes`` follows the same shape because the user-facing +identity is the ``(transaction_id, entity_kind, entity_id, sequence)`` +composite unique key, not the row id; the API surfaces a deterministic +UUIDv5 ``version_uuid`` derived from ``entity.uuid`` and +``transaction_id`` for stable external references. + +Revision ID: 56cd24c07170 +Revises: 3a8e6f2c1b95 +Create Date: 2026-05-28 19:50:00.000000 + +""" + +from __future__ import annotations + +import sqlalchemy as sa +from alembic import op +from sqlalchemy_utils import UUIDType + +from superset.utils.core import MediumText + +revision = "56cd24c07170" +# Stacked on the current migration head, which is downstream of the composite-PK +# association-tables change (2bee73611e32). That ordering is required: the +# Continuum shadow tables this migration creates mirror the composite-PK shape +# of the live association tables, so composite-PK must already have run. +down_revision = "3a8e6f2c1b95" + + +def upgrade() -> None: + bind = op.get_bind() + + # ------------------------------------------------------------------ + # version_transaction + # + # Audit log for each versioning event. Continuum emits + # ``nextval('version_transaction_id_seq')`` on every INSERT, so the + # sequence must exist before the table on Postgres. SQLite/MySQL + # ignore the explicit CREATE SEQUENCE (they auto-increment natively). + # ------------------------------------------------------------------ + if bind.dialect.name == "postgresql": + op.execute("CREATE SEQUENCE IF NOT EXISTS version_transaction_id_seq") + + op.create_table( + "version_transaction", + sa.Column( + "id", + sa.BigInteger(), + sa.Sequence("version_transaction_id_seq"), + primary_key=True, + autoincrement=True, + nullable=False, + ), + sa.Column("issued_at", sa.DateTime(), nullable=True), + sa.Column("remote_addr", sa.String(50), nullable=True), + sa.Column("user_id", sa.Integer(), nullable=True), + # ``action_kind`` carries the high-level avenue that produced + # this transaction (``restore`` / ``import`` / ``clone``). + # ``NULL`` is the default "ordinary save" — most rows leave + # this empty. Commands set + # ``session.info["_versioning_action_kind"]`` before commit; + # the change-record listener stamps the value here. Parallel + # to ``version_changes.entity_kind`` and ``version_changes.kind`` + # — the schema's third ``*_kind`` column, at transaction scope. + sa.Column("action_kind", sa.String(32), nullable=True), + ) + + if bind.dialect.name == "postgresql": + op.execute( + "ALTER SEQUENCE version_transaction_id_seq OWNED BY version_transaction.id" + ) + + # ------------------------------------------------------------------ + # dashboards_version + # ------------------------------------------------------------------ + op.create_table( + "dashboards_version", + sa.Column("uuid", UUIDType(binary=True), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("dashboard_title", sa.String(500), nullable=True), + # ``MediumText()`` mirrors the live column type — on MySQL plain + # ``TEXT`` caps at 64 KB, which large dashboards exceed; an + # oversized live write would then fail the shadow INSERT under + # ``STRICT_TRANS_TABLES`` (or silently truncate without it) and + # corrupt the history. Postgres ``TEXT`` is unbounded and SQLite + # ignores the length annotation so this is MySQL-driven. + sa.Column("position_json", MediumText(), nullable=True), + sa.Column("description", sa.Text(), nullable=True), + sa.Column("css", MediumText(), nullable=True), + sa.Column("theme_id", sa.Integer(), nullable=True), + sa.Column("certified_by", sa.Text(), nullable=True), + sa.Column("certification_details", sa.Text(), nullable=True), + sa.Column("json_metadata", MediumText(), nullable=True), + sa.Column("slug", sa.String(255), nullable=True), + sa.Column("published", sa.Boolean(), nullable=True), + sa.Column("is_managed_externally", sa.Boolean(), nullable=True), + sa.Column("external_url", sa.Text(), nullable=True), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint("id", "transaction_id"), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_dashboards_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_dashboards_version_end_transaction_id", + ), + ) + op.create_index( + "ix_dashboards_version_end_transaction_id", + "dashboards_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_dashboards_version_operation_type", + "dashboards_version", + ["operation_type"], + ) + op.create_index( + "ix_dashboards_version_transaction_id", + "dashboards_version", + ["transaction_id"], + ) + + # ------------------------------------------------------------------ + # slices_version (Charts) + # ------------------------------------------------------------------ + op.create_table( + "slices_version", + sa.Column("uuid", UUIDType(binary=True), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("slice_name", sa.String(250), nullable=True), + sa.Column("datasource_id", sa.Integer(), nullable=True), + sa.Column("datasource_type", sa.String(200), nullable=True), + sa.Column("datasource_name", sa.String(2000), nullable=True), + sa.Column("viz_type", sa.String(250), nullable=True), + sa.Column("params", MediumText(), nullable=True), + sa.Column("description", sa.Text(), nullable=True), + sa.Column("cache_timeout", sa.Integer(), nullable=True), + sa.Column("certified_by", sa.Text(), nullable=True), + sa.Column("certification_details", sa.Text(), nullable=True), + sa.Column("is_managed_externally", sa.Boolean(), nullable=True), + sa.Column("external_url", sa.Text(), nullable=True), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint("id", "transaction_id"), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_slices_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_slices_version_end_transaction_id", + ), + ) + op.create_index( + "ix_slices_version_end_transaction_id", + "slices_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_slices_version_operation_type", + "slices_version", + ["operation_type"], + ) + op.create_index( + "ix_slices_version_transaction_id", + "slices_version", + ["transaction_id"], + ) + + # ------------------------------------------------------------------ + # tables_version (SqlaTable / Datasets) + # ------------------------------------------------------------------ + op.create_table( + "tables_version", + sa.Column("uuid", UUIDType(binary=True), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("description", sa.Text(), nullable=True), + sa.Column("default_endpoint", sa.Text(), nullable=True), + sa.Column("is_featured", sa.Boolean(), nullable=True), + sa.Column("filter_select_enabled", sa.Boolean(), nullable=True), + sa.Column("offset", sa.Integer(), nullable=True), + sa.Column("cache_timeout", sa.Integer(), nullable=True), + sa.Column("params", sa.String(1000), nullable=True), + sa.Column("is_managed_externally", sa.Boolean(), nullable=True), + sa.Column("external_url", sa.Text(), nullable=True), + sa.Column("table_name", sa.String(250), nullable=True), + sa.Column("main_dttm_col", sa.String(250), nullable=True), + sa.Column("currency_code_column", sa.String(250), nullable=True), + sa.Column("database_id", sa.Integer(), nullable=True), + sa.Column("fetch_values_predicate", sa.Text(), nullable=True), + sa.Column("schema", sa.String(255), nullable=True), + sa.Column("catalog", sa.String(256), nullable=True), + sa.Column("sql", MediumText(), nullable=True), + sa.Column("is_sqllab_view", sa.Boolean(), nullable=True), + sa.Column("template_params", sa.Text(), nullable=True), + sa.Column("extra", sa.Text(), nullable=True), + sa.Column("normalize_columns", sa.Boolean(), nullable=True), + sa.Column("always_filter_main_dttm", sa.Boolean(), nullable=True), + sa.Column("folders", sa.JSON(), nullable=True), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint("id", "transaction_id"), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_tables_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_tables_version_end_transaction_id", + ), + ) + op.create_index( + "ix_tables_version_end_transaction_id", + "tables_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_tables_version_operation_type", + "tables_version", + ["operation_type"], + ) + op.create_index( + "ix_tables_version_transaction_id", + "tables_version", + ["transaction_id"], + ) + + # ------------------------------------------------------------------ + # version_changes + # + # Field-level diff log keyed to a (transaction, entity) pair. Each + # row describes one atomic change (one field or one child-collection + # element) that occurred to one entity during a save. + # + # ``(entity_kind, entity_id)`` is a polymorphic reference: depending + # on ``entity_kind`` (``"chart"`` / ``"dashboard"`` / ``"dataset"``) + # the ``entity_id`` is the integer PK on ``slices`` / ``dashboards`` / + # ``tables`` respectively. SQL has no native polymorphic FK, so the + # constraint is intentionally omitted — cleanup relies on the + # ``CASCADE`` from ``version_transaction.id`` plus command-layer + # ordering for entity deletes (the command that hard-deletes the + # entity runs inside the same transaction that prunes its history). + # A bare ``DELETE FROM <entity_table> WHERE id = X`` outside that + # transactional boundary leaves orphan ``version_changes`` rows + # whose ``entity_id`` references a vanished row — the read-side + # tombstone-state lookup handles this gracefully. + # ------------------------------------------------------------------ + op.create_table( + "version_changes", + sa.Column( + "id", + sa.BigInteger(), + primary_key=True, + autoincrement=True, + nullable=False, + ), + sa.Column( + "transaction_id", + sa.BigInteger(), + sa.ForeignKey("version_transaction.id", ondelete="CASCADE"), + nullable=False, + ), + sa.Column( + "entity_kind", + sa.String(length=32), + nullable=False, + ), + sa.Column( + "entity_id", + sa.Integer(), + nullable=False, + ), + sa.Column( + # Integer, not SmallInteger: per-entity sequence within one + # transaction is assigned by unbounded enumerate(); a + # pathological diff (e.g. a giant position_json rewrite) could + # overflow SmallInteger's 32767 on Postgres/MySQL. + "sequence", + sa.Integer(), + nullable=False, + ), + sa.Column( + "kind", + sa.String(length=32), + nullable=False, + ), + # ``operation`` is the per-record verb: ``add`` / ``remove`` / + # ``move`` / ``edit``. ``move`` only fires for layout records; + # the other three apply across every emit site. Made explicit + # so consumers don't have to infer the verb from ``from_value`` + # / ``to_value`` null-tests or from ``path[0]`` for layout records. + sa.Column( + "operation", + sa.String(length=16), + nullable=False, + ), + sa.Column("path", sa.JSON(), nullable=False), + sa.Column("from_value", sa.JSON(), nullable=True), + sa.Column("to_value", sa.JSON(), nullable=True), + sa.UniqueConstraint( + "transaction_id", + "entity_kind", + "entity_id", + "sequence", + name="uq_version_changes_tx_entity_sequence", + ), + ) + op.create_index( + "ix_version_changes_kind", + "version_changes", + ["kind"], + ) + # Leads with the equality/IN prefix (entity_kind, entity_id) and + # extends to transaction_id so the activity read + # (entity_kind = ? AND entity_id IN (...) AND transaction_id >= ?) + # is served by the index rather than falling back to a scan + filesort. + op.create_index( + "ix_version_changes_entity", + "version_changes", + ["entity_kind", "entity_id", "transaction_id"], + ) + + # ------------------------------------------------------------------ + # table_columns_version + # ------------------------------------------------------------------ + op.create_table( + "table_columns_version", + sa.Column("uuid", UUIDType(binary=True), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("column_name", sa.String(255), nullable=True), + sa.Column("verbose_name", sa.String(1024), nullable=True), + sa.Column("is_active", sa.Boolean(), nullable=True), + sa.Column("type", sa.Text(), nullable=True), + sa.Column("advanced_data_type", sa.String(255), nullable=True), + sa.Column("groupby", sa.Boolean(), nullable=True), + sa.Column("filterable", sa.Boolean(), nullable=True), + sa.Column("description", MediumText(), nullable=True), + sa.Column("table_id", sa.Integer(), nullable=True), + sa.Column("is_dttm", sa.Boolean(), nullable=True), + sa.Column("expression", MediumText(), nullable=True), + sa.Column("python_date_format", sa.String(255), nullable=True), + sa.Column("datetime_format", sa.String(100), nullable=True), + sa.Column("extra", sa.Text(), nullable=True), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint("id", "transaction_id"), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_table_columns_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_table_columns_version_end_transaction_id", + ), + ) + op.create_index( + "ix_table_columns_version_end_transaction_id", + "table_columns_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_table_columns_version_operation_type", + "table_columns_version", + ["operation_type"], + ) + op.create_index( + "ix_table_columns_version_transaction_id", + "table_columns_version", + ["transaction_id"], + ) + + # ------------------------------------------------------------------ + # sql_metrics_version + # ------------------------------------------------------------------ + op.create_table( + "sql_metrics_version", + sa.Column("uuid", UUIDType(binary=True), nullable=True), + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("metric_name", sa.String(255), nullable=True), + sa.Column("verbose_name", sa.String(1024), nullable=True), + sa.Column("metric_type", sa.String(32), nullable=True), + sa.Column("description", MediumText(), nullable=True), + sa.Column("d3format", sa.String(128), nullable=True), + sa.Column("currency", sa.JSON(), nullable=True), + sa.Column("warning_text", sa.Text(), nullable=True), + sa.Column("table_id", sa.Integer(), nullable=True), + sa.Column("expression", MediumText(), nullable=True), + sa.Column("extra", sa.Text(), nullable=True), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint("id", "transaction_id"), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_sql_metrics_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_sql_metrics_version_end_transaction_id", + ), + ) + op.create_index( + "ix_sql_metrics_version_end_transaction_id", + "sql_metrics_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_sql_metrics_version_operation_type", + "sql_metrics_version", + ["operation_type"], + ) + op.create_index( + "ix_sql_metrics_version_transaction_id", + "sql_metrics_version", + ["transaction_id"], + ) + + # ------------------------------------------------------------------ + # dashboard_slices_version (M2M association) + # + # The live ``dashboard_slices`` table is reshaped to a + # composite PK on ``(dashboard_id, slice_id)`` — no surrogate ``id``. + # Continuum auto-mirrors the live columns into the shadow Table at + # ``make_versioned()`` time, so the shadow's SQLAlchemy metadata + # also has no ``id``. The DB shadow PK is the natural composite key + # plus Continuum's bookkeeping (``transaction_id``, ``operation_type``); + # ``operation_type`` is included because a single transaction can in + # principle produce both INSERT and DELETE shadows for the same + # ``(dashboard_id, slice_id)`` pair (slice removed and re-added in + # one save). + # + # If that reshape is removed from the stack, the live table reverts to + # carrying its surrogate ``id`` and this migration would need to + # match. + # ------------------------------------------------------------------ + op.create_table( + "dashboard_slices_version", + sa.Column("dashboard_id", sa.Integer(), nullable=False), + sa.Column("slice_id", sa.Integer(), nullable=False), + sa.Column("transaction_id", sa.BigInteger(), nullable=False), + sa.Column("end_transaction_id", sa.BigInteger(), nullable=True), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint( + "dashboard_id", "slice_id", "transaction_id", "operation_type" + ), + sa.ForeignKeyConstraint( + ["transaction_id"], + ["version_transaction.id"], + name="fk_dashboard_slices_version_transaction_id", + ), + sa.ForeignKeyConstraint( + ["end_transaction_id"], + ["version_transaction.id"], + name="fk_dashboard_slices_version_end_transaction_id", + ), + ) + op.create_index( + "ix_dashboard_slices_version_end_transaction_id", + "dashboard_slices_version", + ["end_transaction_id"], + ) + op.create_index( + "ix_dashboard_slices_version_operation_type", + "dashboard_slices_version", + ["operation_type"], + ) + op.create_index( + "ix_dashboard_slices_version_transaction_id", + "dashboard_slices_version", + ["transaction_id"], + ) + + +def downgrade() -> None: + # Drop in reverse dependency order: children with FKs to + # ``version_transaction`` drop first; ``version_transaction`` and its + # sequence drop last. + op.drop_table("dashboard_slices_version") Review Comment: **Suggestion:** Use the shared `drop_table` helper from `superset.migrations.shared.utils` in downgrade logic instead of direct `op.drop_table` calls. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The downgrade path uses raw `op.drop_table` calls even though the shared migration utilities module provides a `drop_table` helper. This is the same raw-operations pattern the rule targets. </details> <details> <summary><b>Rule source 📖 </b></summary> .github/copilot-instructions.md (line 294) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0dac4f9fad54ebcaef67c1055915631&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0dac4f9fad54ebcaef67c1055915631&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py **Line:** 555:555 **Comment:** *Custom Rule: Use the shared `drop_table` helper from `superset.migrations.shared.utils` in downgrade logic instead of direct `op.drop_table` calls. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=7aad0eee3e9ef7d254aaf4cf8919616e412f0bba3f838b361b46d66c91f6ceef&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=7aad0eee3e9ef7d254aaf4cf8919616e412f0bba3f838b361b46d66c91f6ceef&reaction=dislike'>👎</a> -- 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]
