codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3500134671
##########
superset/dashboards/api.py:
##########
@@ -828,17 +886,32 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(Dashboard, pk)
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable so the version-info object type is declared. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code that introduces a local variable without an
explicit type annotation, and the rule requires type hints for relevant
variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=922bed40d98c4f2bbe0d4927930e3bff&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=922bed40d98c4f2bbe0d4927930e3bff&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/dashboards/api.py
**Line:** 892:892
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable so the version-info object type is declared.
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%2F41075&comment_hash=5cec24b1772f24ae6fd53b628129f3fd99f55ae02ff4930d829ffd60569db201&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=5cec24b1772f24ae6fd53b628129f3fd99f55ae02ff4930d829ffd60569db201&reaction=dislike'>👎</a>
##########
superset/dashboards/api.py:
##########
@@ -828,17 +886,32 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(Dashboard, pk)
+
try:
changed_model = UpdateDashboardCommand(pk, item).run()
last_modified_time = changed_model.changed_on.replace(
microsecond=0
).timestamp()
+ new_info = current_entity_version_info(
+ Dashboard, changed_model.id, changed_model.uuid
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable to keep type information complete in modified code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added variable is also unannotated in modified Python code, so it
matches the rule requiring type hints for annotatable variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a1f5bee408f54d248897bf6a3a852b2a&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=a1f5bee408f54d248897bf6a3a852b2a&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/dashboards/api.py
**Line:** 899:901
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable to keep type information complete in modified code.
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%2F41075&comment_hash=0ccf747f9223d20571ada3211639c2a02a322fe9352c3ef2cda6990145516b92&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=0ccf747f9223d20571ada3211639c2a02a322fe9352c3ef2cda6990145516b92&reaction=dislike'>👎</a>
##########
superset/initialization/__init__.py:
##########
@@ -619,6 +619,229 @@ def init_extensions(self) -> None:
extension.manifest.id,
)
+ @staticmethod
+ def _remove_continuum_write_listeners() -> None:
+ """Detach SQLAlchemy-Continuum's own write listeners.
+
+ ``make_versioned()`` runs unconditionally at import of
+ ``superset.extensions`` and registers Continuum's mapper, session,
+ and engine listeners — the ones that write shadow rows and
+ ``version_transaction`` rows on every flush. Skipping only the
+ custom baseline/change-record listeners would leave those running,
+ so with the kill-switch off the shadow tables would silently keep
+ accumulating, contradicting the documented contract.
+
+ This is deliberately a *targeted subset* of
+ ``sqlalchemy_continuum.remove_versioning()``: that helper also
+ calls ``manager.reset()``, which clears ``version_class_map`` —
+ and ``version_class()`` would then silently return the live model
+ class, breaking the read-only ``/versions/`` endpoints this flag
+ promises to keep working.
+
+ Idempotent: guarded on a representative listener so repeated app
+ initializations in one process (test fixtures) don't raise on
+ double-removal.
+ """
+ # pylint: disable=import-outside-toplevel
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ if not sa.event.contains(
+ sa.orm.Mapper, "after_insert", versioning_manager.track_inserts
+ ):
+ return # already detached by a prior init
+ versioning_manager.remove_operations_tracking(sa.orm.Mapper)
+ versioning_manager.remove_session_tracking(sa.orm.session.Session)
+ sa.event.remove(
+ sa.engine.Engine,
+ "before_execute",
+ versioning_manager.track_association_operations,
+ )
+ sa.event.remove(
+ sa.engine.Engine, "rollback", versioning_manager.clear_connection
+ )
+ sa.event.remove(
+ sa.engine.Engine,
+ "set_connection_execution_options",
+ versioning_manager.track_cloned_connections,
+ )
+
+ # Belt-and-suspenders: flip Continuum's master option off as well.
+ # Every write listener checks ``manager.options['versioning']`` before
+ # doing work (manager.py / unit_of_work.py), so if a future Continuum
+ # version registers an additional write listener this detach does not
+ # know to remove, that listener still no-ops. ``version_class()`` reads
+ # from ``version_class_map`` and ignores this option, so the read-only
+ # ``/versions/`` endpoints are unaffected.
+ versioning_manager.options["versioning"] = False
+
+ # Verify the known write listeners are actually gone. A Continuum
+ # upgrade that renamed a handler would make the removals above silently
+ # miss, leaving capture half-on while we report "disabled"; surface
+ # that rather than booting in a contradictory state.
+ if sa.event.contains(
+ sa.orm.Mapper, "after_insert", versioning_manager.track_inserts
+ ):
+ logger.warning(
+ "versioning: Continuum write listeners still attached after "
+ "detach; capture may not be fully disabled. This usually means
"
+ "the pinned sqlalchemy-continuum version changed how it "
+ "registers listeners."
+ )
+
+ def init_versioning(self) -> None:
+ """Register SQLAlchemy-Continuum baseline and retention listeners.
+
+ Must be called after all versioned model classes have been imported so
+ that VERSIONED_MODELS can be populated and configure_mappers() has run.
+
+ ``ENABLE_VERSIONING_CAPTURE`` (ships default ``False``) gates the two
+ before-flush listener registrations. The flag is operational, not
+ feature: with it off the infrastructure is inert (no save writes
+ shadow rows); flipping it on activates capture. The switch also lets
+ an operator who observes a versioning-induced regression (e.g. a
+ save-path slowdown attributable to the change-record listener)
+ disable capture in ``superset_config.py`` and restart workers — a
+ 30-second recovery instead of revert-and-redeploy. Shadow tables
+ already created by the migration stay; they just stop accumulating
+ new rows.
+
+ The fallback here is ``False`` so that any app-factory path that
+ does not load ``superset.config`` (some test factories, embedded
+ use) stays inert by default rather than silently enabling capture.
+ """
+ # Beat-schedule check first: the retention task is independent of
+ # save-path capture and remains useful for ageing-out rows already
+ # written by prior deploys. An operator hitting the kill-switch in
+ # anger may also be running a hand-rolled ``CeleryConfig`` that
+ # silently dropped the prune entry; surfacing both misconfigurations
+ # at the same restart is the cheap, observability-positive shape.
+ self._warn_if_retention_beat_missing()
+
+ if not self.config.get("ENABLE_VERSIONING_CAPTURE", False):
+ logger.warning(
+ "versioning: ENABLE_VERSIONING_CAPTURE is False; "
+ "skipping baseline + change-record listener registration "
+ "and detaching Continuum's write listeners. Save-path "
+ "capture is disabled; existing shadow tables and "
+ "/versions/ endpoints continue to work read-only."
+ )
+ self._remove_continuum_write_listeners()
+ return
+
+ # Symmetric with the OFF branch's ``options['versioning'] = False``:
+ # re-assert it on here so capture is restored even if a prior app
+ # init in the same process (multi-app / test reentrancy) flipped the
+ # process-global Continuum option off. Without this, an OFF app
+ # initialized before an ON app would leave the option False and the
+ # baseline listener — which gates on it — would silently write no
+ # baselines despite capture being "enabled".
+ from sqlalchemy_continuum import versioning_manager
+
+ versioning_manager.options["versioning"] = True
+
+ from sqlalchemy.orm import Session # noqa: F401
+ from sqlalchemy_continuum import version_class
+
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+ from superset.versioning.baseline import (
+ register_baseline_listener,
+ VERSIONED_MODELS,
+ )
+
+ # Note: previously this block called ``configure_mappers()`` before
+ # importing the snapshot modules, believing their Table declarations
+ # needed ``version_transaction`` to exist. That's not actually the
+ # case — the snapshot tables reference ``version_transaction.id``
+ # only at the DB level (via the migration); the SQLAlchemy Table
+ # objects here intentionally declare ``transaction_id`` as a plain
+ # ``BigInteger`` without a FK to avoid the resolution dependency.
+ # Removing the global ``configure_mappers()`` avoids eagerly
+ # resolving relationships in other unrelated models (notably
+ # Flask-AppBuilder's AuditMixin on classes like Tag, whose
+ # ``created_by`` primaryjoin only resolves under specific class
+ # registry states in SQLAlchemy 1.4).
+ from superset.versioning.changes import ( # noqa: E402
+ register_change_record_listener,
+ )
+
+ # All versioned models — Dashboard / Slice / SqlaTable plus their
+ # children (TableColumn / SqlMetric) and the dashboard_slices
+ # M2M — go through Continuum's shadow tables. The JSON-snapshot
+ # path that previously backed dataset / dashboard child diffs
+ # has been removed (full-Continuum spike).
+ for model_cls in (Dashboard, Slice, SqlaTable):
+ try:
+ version_class(model_cls) # ensure Continuum wired this model
+ # Dedup guard: VERSIONED_MODELS is module-level state, and
+ # test fixtures initialize multiple Superset apps per
+ # process — without the check each re-init appends
+ # duplicate entries.
+ if model_cls not in VERSIONED_MODELS:
+ VERSIONED_MODELS.append(model_cls)
+ except Exception: # pylint: disable=broad-except
+ # Continuum failed to wire versioning for this model. We
+ # boot in degraded mode rather than failing startup, but a
+ # silent skip would hide that change capture has stopped for
+ # the model — so surface it at WARNING with the traceback.
+ logger.warning(
+ "Versioning is not wired for %s; change capture will be "
+ "skipped for it. This usually means Continuum did not "
+ "register a version class for the model.",
+ model_cls.__name__,
+ exc_info=True,
+ )
+
+ register_baseline_listener()
+ register_change_record_listener()
+
+ # Retention is time-based and runs out-of-band as a Celery beat
+ # task — see ``superset/tasks/version_history_retention.py``
+ # and the ``version_history.prune_old_versions`` entry in
+ # ``CELERYBEAT_SCHEDULE`` (``superset/config.py``). The previous
+ # synchronous after_commit listener was retired so retention
+ # work doesn't add latency to user saves.
+
+ _RETENTION_TASK_NAME = "version_history.prune_old_versions"
Review Comment:
**Suggestion:** Add an explicit type annotation to this new class-level
constant so the new variable in this change is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new class-level constant in modified Python code and it has no
type annotation, so it violates the type-hint rule for relevant variables that
can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea7dc81acb7c4e789f718a4ae86bf4d7&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=ea7dc81acb7c4e789f718a4ae86bf4d7&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/initialization/__init__.py
**Line:** 807:807
**Comment:**
*Custom Rule: Add an explicit type annotation to this new class-level
constant so the new variable in this change is fully typed.
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%2F41075&comment_hash=d7290601bc19e9a44c8a5a459aa730186efb4be6f4d3e6044a52a72b33d4e807&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=d7290601bc19e9a44c8a5a459aa730186efb4be6f4d3e6044a52a72b33d4e807&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+"""composite_pk_association_tables
+
+Replace the unused synthetic ``id INTEGER PRIMARY KEY`` on eight many-to-many
+association tables with a composite primary key on the two FK columns. Drops
+the now-redundant ``UniqueConstraint(fk1, fk2)`` on the two tables that
+already carry one. Pre-flight: deletes rows with NULL FK values (six tables
+allow them today) and any duplicate ``(fk1, fk2)`` rows.
+
+Motivated by SQLAlchemy-Continuum issue #129 (M2M restore against junction
+tables with surrogate PKs); also closes the data-integrity hole where six
+of the eight tables lacked DB-level uniqueness.
+
+Revision ID: 2bee73611e32
+Revises: 78a40c08b4be
+Create Date: 2026-05-01 23:36:34.050058
+
+"""
+
+import logging
+from typing import NamedTuple
+
+import sqlalchemy as sa
+from alembic import op
+from alembic.operations.base import BatchOperations
+from sqlalchemy import inspect
+from sqlalchemy.engine import Connection
+
+# revision identifiers, used by Alembic.
+revision = "2bee73611e32"
+down_revision = "78a40c08b4be"
Review Comment:
**Suggestion:** Add explicit type annotations to these module-level revision
identifiers to comply with the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
These module-level revision identifiers are unannotated, and they are
relevant variables that can be type-hinted. That matches the Python type-hint
requirement for new or modified code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=77fdcb7693874e4a88c058af8712733d&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=77fdcb7693874e4a88c058af8712733d&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-01_23-36_2bee73611e32_composite_pk_association_tables.py
**Line:** 45:46
**Comment:**
*Custom Rule: Add explicit type annotations to these module-level
revision identifiers to comply with the type-hint requirement for relevant
variables.
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%2F41075&comment_hash=7ba9a389778afd20768ecc3023935f2983f3b8b8482bbc3bfbe70ee7da310dff&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=7ba9a389778afd20768ecc3023935f2983f3b8b8482bbc3bfbe70ee7da310dff&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py:
##########
@@ -0,0 +1,173 @@
+# 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.
+"""shadow_live_row_indexes
+
+Adds per-shadow-table indexes covering the canonical "current live row
+of entity X" lookup that ``find_active_by_uuid`` / ``list_versions`` /
+``get_version`` / restore validation / activity-view all funnel
+through:
+
+ SELECT ... FROM <entity>_version
+ WHERE id = ? AND end_transaction_id IS NULL
+
+The base migration (``56cd24c07170_add_versioning_tables``) created
+single-column indexes on ``transaction_id``, ``end_transaction_id``,
+and ``operation_type``, but nothing covering the predicate combination
+that actually runs in hot paths.
+
+Index choice is dialect-specific:
+
+* **PostgreSQL / SQLite** — partial index over the entity ``id`` with
+ ``WHERE end_transaction_id IS NULL``. Cuts the index size to one row
+ per live entity (vs. one row per historical version) and turns the
+ hot lookup into a single index probe.
+* **MySQL** — partial indexes aren't supported; use a plain composite
+ ``(id, end_transaction_id)``. MySQL's optimizer handles the
+ ``IS NULL`` predicate against the composite efficiently.
+
+It also adds a composite ``(table_id, transaction_id)`` index on the two
+child shadow tables (``table_columns_version`` / ``sql_metrics_version``).
+The dataset child-diff path queries these by parent ``table_id`` plus a
+transaction-range bound, neither of which the base migration's
+single-column indexes nor the ``id``-leading PK can serve:
+
+ SELECT ... FROM table_columns_version
+ WHERE table_id = ? AND transaction_id <= ? AND ... (shadow_rows_valid_at)
+
+ SELECT max(transaction_id) FROM table_columns_version
+ WHERE table_id = ? AND transaction_id < ? (prior-tx probe)
+
+A plain composite leading with ``table_id`` serves both on every dialect,
+so no partial-index split is needed here.
+
+Surfaced by sqlalchemy-review pass W-NEW-4 (live-row lookup) and a
+Codex sqlalchemy-review pass (child-diff ``table_id`` lookup).
+
+Revision ID: 8f3a1b2c4d5e
+Revises: 56cd24c07170
+Create Date: 2026-06-03 12:00:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "8f3a1b2c4d5e"
+down_revision = "56cd24c07170"
+
+
+# The parent + child shadow tables, all of which carry an ``id``
+# column (mirroring the live entity's integer PK). ``dashboard_slices_version``
+# is intentionally excluded: it's the M2M association shadow with a
+# composite PK ``(dashboard_id, slice_id, transaction_id, operation_type)``
+# and no ``id`` column. The canonical "live row" lookup doesn't apply to
+# the M2M shadow — readers query it by ``transaction_id`` (already
+# indexed by the base migration) when reconstructing per-tx changes.
+SHADOW_TABLES: tuple[str, ...] = (
+ "dashboards_version",
+ "slices_version",
+ "tables_version",
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+# Child shadow tables whose rows are looked up by parent ``table_id`` plus a
+# transaction-range bound on the dataset child-diff path. Both carry a
+# nullable ``table_id`` mirroring the live row's FK to ``tables.id``.
+CHILD_SHADOW_TABLES: tuple[str, ...] = (
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+def _index_name(table: str) -> str:
+ return f"ix_{table}_live_id"
+
+
+def _child_index_name(table: str) -> str:
+ return f"ix_{table}_table_id_transaction_id"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ dialect = bind.dialect.name
+
+ where_clause = sa.text("end_transaction_id IS NULL")
+
+ for table in SHADOW_TABLES:
+ index_name = _index_name(table)
+ if dialect == "postgresql":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ postgresql_where=where_clause,
+ )
+ elif dialect == "sqlite":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ sqlite_where=where_clause,
+ )
+ else:
+ # MySQL (and any unknown dialect): partial indexes aren't
+ # supported, so use a plain composite. MySQL's optimizer
+ # handles ``id = ? AND end_transaction_id IS NULL`` against
+ # the composite efficiently.
+ op.create_index(
+ index_name,
+ table,
+ ["id", "end_transaction_id"],
+ unique=False,
+ )
Review Comment:
**Suggestion:** For the non-partial index branch, switch from raw Alembic
index creation to the shared migration utility helper. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a migration file using raw op.create_index for a plain composite
index even though superset.migrations.shared.utils.create_index exists for
compatibility-aware index creation. That is a real instance of the stated
migration-helper rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc6f956536524f9db8dc75a7ad347791&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=cc6f956536524f9db8dc75a7ad347791&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-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py
**Line:** 132:142
**Comment:**
*Custom Rule: For the non-partial index branch, switch from raw Alembic
index creation to the shared migration utility helper.
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%2F41075&comment_hash=2ea7f07ac08d97c5d6071e7cef290ebe540bea12ac724bdb8b4151fef3e42b98&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=2ea7f07ac08d97c5d6071e7cef290ebe540bea12ac724bdb8b4151fef3e42b98&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py:
##########
@@ -0,0 +1,173 @@
+# 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.
+"""shadow_live_row_indexes
+
+Adds per-shadow-table indexes covering the canonical "current live row
+of entity X" lookup that ``find_active_by_uuid`` / ``list_versions`` /
+``get_version`` / restore validation / activity-view all funnel
+through:
+
+ SELECT ... FROM <entity>_version
+ WHERE id = ? AND end_transaction_id IS NULL
+
+The base migration (``56cd24c07170_add_versioning_tables``) created
+single-column indexes on ``transaction_id``, ``end_transaction_id``,
+and ``operation_type``, but nothing covering the predicate combination
+that actually runs in hot paths.
+
+Index choice is dialect-specific:
+
+* **PostgreSQL / SQLite** — partial index over the entity ``id`` with
+ ``WHERE end_transaction_id IS NULL``. Cuts the index size to one row
+ per live entity (vs. one row per historical version) and turns the
+ hot lookup into a single index probe.
+* **MySQL** — partial indexes aren't supported; use a plain composite
+ ``(id, end_transaction_id)``. MySQL's optimizer handles the
+ ``IS NULL`` predicate against the composite efficiently.
+
+It also adds a composite ``(table_id, transaction_id)`` index on the two
+child shadow tables (``table_columns_version`` / ``sql_metrics_version``).
+The dataset child-diff path queries these by parent ``table_id`` plus a
+transaction-range bound, neither of which the base migration's
+single-column indexes nor the ``id``-leading PK can serve:
+
+ SELECT ... FROM table_columns_version
+ WHERE table_id = ? AND transaction_id <= ? AND ... (shadow_rows_valid_at)
+
+ SELECT max(transaction_id) FROM table_columns_version
+ WHERE table_id = ? AND transaction_id < ? (prior-tx probe)
+
+A plain composite leading with ``table_id`` serves both on every dialect,
+so no partial-index split is needed here.
+
+Surfaced by sqlalchemy-review pass W-NEW-4 (live-row lookup) and a
+Codex sqlalchemy-review pass (child-diff ``table_id`` lookup).
+
+Revision ID: 8f3a1b2c4d5e
+Revises: 56cd24c07170
+Create Date: 2026-06-03 12:00:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "8f3a1b2c4d5e"
+down_revision = "56cd24c07170"
+
+
+# The parent + child shadow tables, all of which carry an ``id``
+# column (mirroring the live entity's integer PK). ``dashboard_slices_version``
+# is intentionally excluded: it's the M2M association shadow with a
+# composite PK ``(dashboard_id, slice_id, transaction_id, operation_type)``
+# and no ``id`` column. The canonical "live row" lookup doesn't apply to
+# the M2M shadow — readers query it by ``transaction_id`` (already
+# indexed by the base migration) when reconstructing per-tx changes.
+SHADOW_TABLES: tuple[str, ...] = (
+ "dashboards_version",
+ "slices_version",
+ "tables_version",
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+# Child shadow tables whose rows are looked up by parent ``table_id`` plus a
+# transaction-range bound on the dataset child-diff path. Both carry a
+# nullable ``table_id`` mirroring the live row's FK to ``tables.id``.
+CHILD_SHADOW_TABLES: tuple[str, ...] = (
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+def _index_name(table: str) -> str:
+ return f"ix_{table}_live_id"
+
+
+def _child_index_name(table: str) -> str:
+ return f"ix_{table}_table_id_transaction_id"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ dialect = bind.dialect.name
+
+ where_clause = sa.text("end_transaction_id IS NULL")
+
+ for table in SHADOW_TABLES:
+ index_name = _index_name(table)
+ if dialect == "postgresql":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ postgresql_where=where_clause,
+ )
+ elif dialect == "sqlite":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ sqlite_where=where_clause,
+ )
+ else:
+ # MySQL (and any unknown dialect): partial indexes aren't
+ # supported, so use a plain composite. MySQL's optimizer
+ # handles ``id = ? AND end_transaction_id IS NULL`` against
+ # the composite efficiently.
+ op.create_index(
+ index_name,
+ table,
+ ["id", "end_transaction_id"],
+ unique=False,
+ )
+
+ # Child-diff access pattern: filter by parent ``table_id`` plus a
+ # transaction-range bound. A plain composite serves this on every
+ # dialect, so no partial-index split is needed.
+ for table in CHILD_SHADOW_TABLES:
+ op.create_index(
+ _child_index_name(table),
+ table,
+ ["table_id", "transaction_id"],
+ unique=False,
+ )
Review Comment:
**Suggestion:** Use the shared migration index helper for this plain
composite index creation instead of calling Alembic operations directly.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The migration shared utils module provides a compatibility wrapper for
creating indexes, and this code calls Alembic's raw op.create_index directly in
a migration file. That matches the rule requiring shared helpers when available.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=896d1861182b435f8faee2dc200062cd&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=896d1861182b435f8faee2dc200062cd&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-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py
**Line:** 147:153
**Comment:**
*Custom Rule: Use the shared migration index helper for this plain
composite index creation instead of calling Alembic operations directly.
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%2F41075&comment_hash=bae8e83f58b328a37624aeb35c55d0d286534dc5a1fd9abe542ab7ff160fe051&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=bae8e83f58b328a37624aeb35c55d0d286534dc5a1fd9abe542ab7ff160fe051&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py:
##########
@@ -0,0 +1,173 @@
+# 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.
+"""shadow_live_row_indexes
+
+Adds per-shadow-table indexes covering the canonical "current live row
+of entity X" lookup that ``find_active_by_uuid`` / ``list_versions`` /
+``get_version`` / restore validation / activity-view all funnel
+through:
+
+ SELECT ... FROM <entity>_version
+ WHERE id = ? AND end_transaction_id IS NULL
+
+The base migration (``56cd24c07170_add_versioning_tables``) created
+single-column indexes on ``transaction_id``, ``end_transaction_id``,
+and ``operation_type``, but nothing covering the predicate combination
+that actually runs in hot paths.
+
+Index choice is dialect-specific:
+
+* **PostgreSQL / SQLite** — partial index over the entity ``id`` with
+ ``WHERE end_transaction_id IS NULL``. Cuts the index size to one row
+ per live entity (vs. one row per historical version) and turns the
+ hot lookup into a single index probe.
+* **MySQL** — partial indexes aren't supported; use a plain composite
+ ``(id, end_transaction_id)``. MySQL's optimizer handles the
+ ``IS NULL`` predicate against the composite efficiently.
+
+It also adds a composite ``(table_id, transaction_id)`` index on the two
+child shadow tables (``table_columns_version`` / ``sql_metrics_version``).
+The dataset child-diff path queries these by parent ``table_id`` plus a
+transaction-range bound, neither of which the base migration's
+single-column indexes nor the ``id``-leading PK can serve:
+
+ SELECT ... FROM table_columns_version
+ WHERE table_id = ? AND transaction_id <= ? AND ... (shadow_rows_valid_at)
+
+ SELECT max(transaction_id) FROM table_columns_version
+ WHERE table_id = ? AND transaction_id < ? (prior-tx probe)
+
+A plain composite leading with ``table_id`` serves both on every dialect,
+so no partial-index split is needed here.
+
+Surfaced by sqlalchemy-review pass W-NEW-4 (live-row lookup) and a
+Codex sqlalchemy-review pass (child-diff ``table_id`` lookup).
+
+Revision ID: 8f3a1b2c4d5e
+Revises: 56cd24c07170
+Create Date: 2026-06-03 12:00:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "8f3a1b2c4d5e"
+down_revision = "56cd24c07170"
+
+
+# The parent + child shadow tables, all of which carry an ``id``
+# column (mirroring the live entity's integer PK). ``dashboard_slices_version``
+# is intentionally excluded: it's the M2M association shadow with a
+# composite PK ``(dashboard_id, slice_id, transaction_id, operation_type)``
+# and no ``id`` column. The canonical "live row" lookup doesn't apply to
+# the M2M shadow — readers query it by ``transaction_id`` (already
+# indexed by the base migration) when reconstructing per-tx changes.
+SHADOW_TABLES: tuple[str, ...] = (
+ "dashboards_version",
+ "slices_version",
+ "tables_version",
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+# Child shadow tables whose rows are looked up by parent ``table_id`` plus a
+# transaction-range bound on the dataset child-diff path. Both carry a
+# nullable ``table_id`` mirroring the live row's FK to ``tables.id``.
+CHILD_SHADOW_TABLES: tuple[str, ...] = (
+ "table_columns_version",
+ "sql_metrics_version",
+)
+
+
+def _index_name(table: str) -> str:
+ return f"ix_{table}_live_id"
+
+
+def _child_index_name(table: str) -> str:
+ return f"ix_{table}_table_id_transaction_id"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ dialect = bind.dialect.name
+
+ where_clause = sa.text("end_transaction_id IS NULL")
+
+ for table in SHADOW_TABLES:
+ index_name = _index_name(table)
+ if dialect == "postgresql":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ postgresql_where=where_clause,
+ )
+ elif dialect == "sqlite":
+ op.create_index(
+ index_name,
+ table,
+ ["id"],
+ unique=False,
+ sqlite_where=where_clause,
+ )
+ else:
+ # MySQL (and any unknown dialect): partial indexes aren't
+ # supported, so use a plain composite. MySQL's optimizer
+ # handles ``id = ? AND end_transaction_id IS NULL`` against
+ # the composite efficiently.
+ op.create_index(
+ index_name,
+ table,
+ ["id", "end_transaction_id"],
+ unique=False,
+ )
+
+ # Child-diff access pattern: filter by parent ``table_id`` plus a
+ # transaction-range bound. A plain composite serves this on every
+ # dialect, so no partial-index split is needed.
+ for table in CHILD_SHADOW_TABLES:
+ op.create_index(
+ _child_index_name(table),
+ table,
+ ["table_id", "transaction_id"],
+ unique=False,
+ )
+
+
+def downgrade() -> None:
+ # Probe the inspector instead of emitting ``DROP INDEX IF EXISTS``:
+ # stock MySQL (5.7/8.x) has no IF EXISTS grammar for DROP INDEX
+ # (it's a MariaDB extension), so the clause is not dialect-portable.
+ # The existence check keeps the downgrade robust against a
+ # partial-application failure on upgrade (e.g. the first
+ # ``op.create_index`` succeeded under Postgres' transactional DDL but
+ # a later one failed and rolled back the rest — repeated downgrade
+ # must not raise on the missing indexes).
+ inspector = sa.inspect(op.get_bind())
+ for table in SHADOW_TABLES:
+ index_name = _index_name(table)
+ if any(ix["name"] == index_name for ix in
inspector.get_indexes(table)):
+ op.drop_index(index_name, table_name=table)
+ for table in CHILD_SHADOW_TABLES:
+ index_name = _child_index_name(table)
+ if any(ix["name"] == index_name for ix in
inspector.get_indexes(table)):
+ op.drop_index(index_name, table_name=table)
Review Comment:
**Suggestion:** Replace manual inspector-based index existence checks and
direct drop calls with the shared migration drop-index helper. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The migration utility module exposes drop_index as a compatibility helper,
but this downgrade uses manual inspector checks plus raw op.drop_index calls.
Since a shared helper is available for portable index removal, this matches the
rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57a0d8748c214f848f1605ed4d54478f&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=57a0d8748c214f848f1605ed4d54478f&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-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py
**Line:** 165:173
**Comment:**
*Custom Rule: Replace manual inspector-based index existence checks and
direct drop calls with the shared migration drop-index helper.
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%2F41075&comment_hash=64337cd7ab6949dde1dcc970fc5b7f96bc209b30a3a53fc45637e1c0431fee8d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=64337cd7ab6949dde1dcc970fc5b7f96bc209b30a3a53fc45637e1c0431fee8d&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]
