codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3560181751
##########
superset/initialization/__init__.py:
##########
@@ -849,10 +857,61 @@ def init_versioning(self) -> None:
register_baseline_listener()
register_change_record_listener()
- # Retention pruning runs out-of-band as a scheduled Celery beat
- # task, shipped as a separate stacked PR. The previous
- # synchronous after_commit listener was retired so retention work
- # doesn't add latency to user saves.
+ # 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
+ # ``CeleryConfig.beat_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 it complies with the type-hint requirement for annotatable
variables. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a newly added class-level constant that can be annotated, but it is
defined without a type hint. That matches the rule requiring type hints on
relevant Python variables that can be annotated.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=59e21ec2d42e45f29e16bd7da5fc37f7&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=59e21ec2d42e45f29e16bd7da5fc37f7&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:** 867:867
**Comment:**
*Custom Rule: Add an explicit type annotation to this new class-level
constant so it complies with the type-hint requirement for annotatable
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=ca45a6cd229ee943fba023e60b0194244e643f79307e4beab7e74e09c6a816e1&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=ca45a6cd229ee943fba023e60b0194244e643f79307e4beab7e74e09c6a816e1&reaction=dislike'>π</a>
##########
superset/tasks/version_history_retention.py:
##########
@@ -0,0 +1,552 @@
+# 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.
+"""Celery task: prune old entity-version history.
+
+Retention is time-based. The task deletes parent + child shadow rows
+owned by ``version_transaction`` rows whose ``issued_at`` is older
+than ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` (default 30, env
+overridable, ``0`` to disable).
+
+One preservation rule, applied across every shadow table (parent,
+child, and the M2M association):
+
+* **Live** (``end_transaction_id IS NULL``) β never pruned.
+
+Baseline rows (``operation_type = 0``) and any closed historical row
+are subject to the same retention window as everything else. An
+entity that hasn't been edited within the window has only its live
+row remaining; the historical chain (including the synthetic
+baseline) ages out.
+
+If any shadow row anchored at a transaction is live (in a parent,
+child, or M2M shadow), the whole transaction is preserved (along with
+its other shadow rows and ``version_changes`` rows). Otherwise, all of
+the transaction's shadow rows are deleted and the ``version_transaction``
+row itself is dropped β its ``version_changes`` rows cascade via the FK.
+
+Registered via ``CeleryConfig.beat_schedule`` in ``superset/config.py``.
+Idempotent: a second run prunes nothing.
+"""
+
+from __future__ import annotations
+
+import logging
+import time
+from collections.abc import Iterator
+from dataclasses import dataclass
+from datetime import datetime, timedelta, timezone
+from typing import Any
+
+import sqlalchemy as sa
+from flask import current_app
+from sqlalchemy.exc import OperationalError
+
+from superset.extensions import celery_app, db, stats_logger_manager
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class ShadowTables:
+ """The four Continuum-managed Table objects the prune walks.
+
+ Bundled here so the prune helper's signature stays at two arguments
+ instead of five. The shape is set once at task entry by
+ ``_resolve_shadow_tables`` and threaded through the retry loop.
+ """
+
+ parent: list[sa.Table]
+ child: list[sa.Table]
+ m2m: sa.Table | None
+ transaction: sa.Table
+
+
+def _resolve_shadow_tables(tx_table: sa.Table) -> ShadowTables:
+ """Resolve the parent / child / m2m shadow Tables from Continuum's
+ mapper registry and bundle them with the transaction Table.
+
+ ``dashboard_slices_version`` is M2M-tracked by Continuum and lives
+ in metadata under that name (Continuum auto-creates the Table; it
+ isn't registered as a versioned class). Carried separately on the
+ ``ShadowTables`` dataclass because it doesn't follow the parent /
+ child class shape.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import version_class
+ from sqlalchemy_continuum.exc import ClassNotVersioned
+
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric,
TableColumn
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ # ``ClassNotVersioned`` is the only expected failure here β versioning
+ # init runs at startup; if it didn't, every class lookup raises this.
+ # Narrowing the catch keeps a real underlying failure (e.g. a metadata
+ # inconsistency after ``make_versioned``) from being silently swallowed
+ # into a no-op retention pass.
+ parent_tables: list[sa.Table] = []
+ for cls in (Dashboard, Slice, SqlaTable):
+ try:
+ parent_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ child_tables: list[sa.Table] = []
+ for cls in (TableColumn, SqlMetric):
+ try:
+ child_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ metadata = parent_tables[0].metadata if parent_tables else None
+ m2m_table = (
+ metadata.tables.get("dashboard_slices_version")
+ if metadata is not None
+ else None
+ )
+
+ return ShadowTables(
+ parent=parent_tables,
+ child=child_tables,
+ m2m=m2m_table,
+ transaction=tx_table,
+ )
+
+
+@dataclass(frozen=True)
+class _PruneWindow:
+ """One id-ordered window of prune candidates.
+
+ ``prunable`` is the subset of the window's candidate transactions
+ that are safe to delete. ``candidate_count`` and ``max_candidate_id``
+ drive the batch loop in :func:`_prune_old_versions_impl`: the loop
+ stops when a window returns fewer than ``_MAX_PRUNE_BATCH``
+ candidates, and otherwise advances ``after_id`` to
+ ``max_candidate_id`` to fetch the next window.
+ """
+
+ prunable: list[int]
+ candidate_count: int
+ max_candidate_id: int
+
+
+def _resolve_prune_window(
+ conn: sa.engine.Connection,
+ cutoff: datetime,
+ shadow_tables: list[sa.Table],
+ after_id: int,
+ batch_size: int,
+) -> _PruneWindow:
+ """Resolve one id-ordered window of ``version_transaction`` rows
+ eligible to prune: ``issued_at < cutoff`` AND ``id > after_id``,
+ ordered by ``id`` and capped at *batch_size*. From that window,
+ ``prunable`` excludes any transaction still serving as the live row
+ (``end_transaction_id IS NULL``) of any versioned entity in *any*
+ shadow table β parent, child, or M2M.
+
+ A child (``table_columns_version`` / ``sql_metrics_version``) or the
+ M2M association (``dashboard_slices_version``) is versioned on a
+ validity lifecycle independent of its parent: after a parent-only
+ edit, an unchanged child stays live anchored at an *older*
+ transaction than the parent's current live row. That older
+ transaction must be preserved too β otherwise
+ :func:`_delete_for_transactions` would drop the still-live
+ child/association row and silently strip the surviving version of its
+ columns / metrics / slices. Scanning every shadow for live rows (not
+ just parents) is what prevents that corruption.
+
+ Windowing by ``id`` (rather than materializing the whole backlog)
+ keeps per-pass memory and lock/transaction-hold time bounded. Live
+ rows older than the cutoff are never deleted but are skipped via the
+ ``after_id`` watermark, so the batch loop still terminates.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ # ``ORDER BY id LIMIT`` drives this off the primary key (clustered on
+ # InnoDB, rowid on SQLite, PK index on Postgres): it satisfies the
+ # ``id > after_id`` pagination and the ordering in one structure, with
+ # early termination and no sort. ``issued_at`` is applied as an inline
+ # filter. Because ids are monotonic with ``issued_at`` (append-only
+ # transaction log), the rows to prune cluster at the low-id end, so the
+ # PK scan is already efficient. A standalone ``issued_at`` index would
+ # force a filesort to satisfy ``ORDER BY id`` and so would not be chosen
+ # by the planner for this query on any supported engine β hence none is
+ # created.
+ candidate_ids = [
+ row[0]
+ for row in conn.execute(
+ sa.select(tx_table.c.id)
+ .where(tx_table.c.issued_at < cutoff)
+ .where(tx_table.c.id > after_id)
+ .order_by(tx_table.c.id)
+ .limit(batch_size)
+ )
+ ]
+ if not candidate_ids:
+ return _PruneWindow(prunable=[], candidate_count=0,
max_candidate_id=after_id)
+
+ # The select is ordered by ``id`` ascending, so the last element is
+ # the watermark for the next window.
+ max_candidate_id = candidate_ids[-1]
+
+ # Build the set of transaction ids that still anchor a live row
+ # (``end_transaction_id IS NULL``) in some shadow table. Those
+ # transactions represent the current state of an entity (or one of
+ # its children / associations) and must be preserved regardless of
+ # age. Chunked over the candidates to keep the bind-parameter count
+ # inside SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` floor (see
+ # ``_TX_ID_CHUNK_SIZE`` below). Ids already confirmed live by an
+ # earlier table are skipped before probing the next one.
+ preserved_ids: set[int] = set()
+ for stbl in shadow_tables:
+ remaining = [tx_id for tx_id in candidate_ids if tx_id not in
preserved_ids]
+ if not remaining:
+ break
+ for chunk in _chunked(remaining, _TX_ID_CHUNK_SIZE):
+ for row in conn.execute(
+ sa.select(stbl.c.transaction_id)
+ .where(stbl.c.transaction_id.in_(chunk))
+ .where(stbl.c.end_transaction_id.is_(None))
+ .distinct()
+ ):
+ preserved_ids.add(row[0])
+
+ prunable = [tx_id for tx_id in candidate_ids if tx_id not in preserved_ids]
+ return _PruneWindow(
+ prunable=prunable,
+ candidate_count=len(candidate_ids),
+ max_candidate_id=max_candidate_id,
+ )
+
+
+# SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` defaults to 999 (lifted to
+# 32766 in 3.32+ but the older limit can still apply in shipped
+# builds). Postgres + MySQL handle tens of thousands of bind params
+# without complaint, so the chunk size is dictated by the SQLite floor.
+# 500 keeps each single-column DELETE / SELECT (see
``_delete_for_transactions``,
+# which splits the create/close predicates into two statements) well inside
+# that floor, with margin for any other bound params in the surrounding
+# statement.
+_TX_ID_CHUNK_SIZE = 500
+
+# Maximum ``version_transaction`` rows resolved + pruned per SERIALIZABLE
+# pass. The prune loops over id-ordered windows of this size (see
+# ``_prune_old_versions_impl``) so memory and lock/transaction-hold time
+# stay bounded per pass instead of scaling with the full backlog on the
+# first run after deploy.
+_MAX_PRUNE_BATCH = 1000
+
+
+def _delete_for_transactions(
+ conn: sa.engine.Connection,
+ tables: list[sa.Table],
+ tx_ids: list[int],
+) -> int:
+ """Delete shadow rows in *tables* whose lifespan touches a pruned
+ transaction β either ``transaction_id`` (created at) or
+ ``end_transaction_id`` (closed at) is in *tx_ids*. Returns total
+ rowcount across all tables.
+
+ The ``end_transaction_id`` predicate is required to keep referential
+ integrity when transactions span multiple entities. A flush that
+ saves dashboard + slice + dataset at the same ``tx=X`` produces
+ three shadow rows sharing that tx. If only the dashboard is later
+ edited at ``tx=Y``, the dashboard row at ``tx=X`` is closed
+ (``end_tx=Y``) while the slice/dataset rows stay live at
+ ``tx=X``. Retention preserves ``tx=X`` (slice/dataset are live
+ there) and prunes ``tx=Y``. Without the ``end_tx`` predicate, the
+ dashboard's closed row at ``tx=X`` survives step 1 β its
+ ``end_transaction_id=Y`` then violates the FK when step 2 deletes
+ ``version_transaction`` row ``Y``.
+
+ Live rows are never matched by either predicate
+ (``end_transaction_id IS NULL`` is not ``IN`` anything; live rows'
+ ``transaction_id`` is preserved by construction in
+ :func:`_resolve_prune_window`).
+
+ ``tx_ids`` is chunked into batches of ``_TX_ID_CHUNK_SIZE`` so the
+ bind-parameter count stays inside SQLite's ``SQLITE_MAX_VARIABLE_
+ NUMBER`` limit. Postgres and MySQL would happily accept the full
+ list, but the floor is dialect-agnostic since the retention task is
+ the only path that accumulates open-ended id batches.
+ """
+ if not tx_ids:
+ return 0
+ total = 0
+ for tbl in tables:
+ for chunk in _chunked(tx_ids, _TX_ID_CHUNK_SIZE):
+ # Two single-column DELETEs rather than one ``OR`` of both
+ # columns. The OR form binds every id twice (once for
+ # ``transaction_id``, once for ``end_transaction_id``), so a
+ # full chunk would bind ``2 * _TX_ID_CHUNK_SIZE`` params and
+ # overflow SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` floor. Each
+ # single-column statement binds at most ``_TX_ID_CHUNK_SIZE``.
+ # A row whose create- and close-tx are both pruned is removed
+ # by the first matching DELETE; the second finds nothing, so
+ # there is no double count.
+ for col in (tbl.c.transaction_id, tbl.c.end_transaction_id):
+ result = conn.execute(sa.delete(tbl).where(col.in_(chunk)))
+ total += result.rowcount or 0
+ return total
+
+
+def _chunked(items: list[int], size: int) -> Iterator[list[int]]:
+ """Yield *items* in fixed-size lists. Final chunk may be smaller."""
+ for i in range(0, len(items), size):
+ yield items[i : i + size]
+
+
+#: Maximum number of attempts the prune will make before giving up.
+#: A daily Celery beat schedule means the next chance is 24h out, so
+#: a small inline retry materially improves the recovery time for the
+#: serialization-conflict path.
+_MAX_RETRY_ATTEMPTS = 3
+
+#: Base for exponential backoff between retries (seconds). Worst-case
+#: extra latency with the 3-attempt cap above and the factor below is
+#: ``BASE + BASE * FACTOR`` = ~0.5s β well inside the prune's own
+#: typical runtime.
+_RETRY_BACKOFF_BASE_SECONDS = 0.1
+
+#: Exponential-backoff multiplier between successive retry attempts.
+#: Backoff for attempt N is ``BASE * (FACTOR ** (N - 1))``.
+_RETRY_BACKOFF_FACTOR = 4
+
+#: Statsd metric prefix for retention emissions. Mirrors the activity-view
+#: orchestrator's ``superset.activity_view.*`` namespace so a single
+#: Grafana filter (``superset.versioning.*``) catches both sides of the
+#: feature. The pruned-count gauge fires every run; the skipped counter
+#: fires for the "retention disabled" and "no versioned classes" cases;
+#: the retried counter fires when the SERIALIZABLE block tripped at
+#: least one conflict before settling.
+_METRIC_PREFIX = "superset.versioning.retention"
+
+
+def _run_prune_pass(
+ cutoff: datetime, tables: ShadowTables, after_id: int = 0
+) -> dict[str, Any]:
+ """One SERIALIZABLE pass over a single id-ordered window of candidate
+ transactions starting after ``after_id``. The caller wraps this in
+ the retry loop (serialization conflict β fresh connection from a
+ clean snapshot) and the batch loop (advance ``after_id`` until the
+ backlog drains). The returned ``candidate_count`` / ``max_candidate_id``
+ drive that batch loop."""
+ # Scan every shadow (parent + child + M2M) for live rows, not just
+ # parents: children and the M2M association live on independent
+ # validity lifecycles and may anchor a still-live row at an older
+ # transaction than the parent's current live row.
+ live_bearing_tables = [*tables.parent, *tables.child]
Review Comment:
**Suggestion:** Add a type annotation to this merged table list variable to
comply with required type hints on relevant locals. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a newly introduced local list variable with an obvious annotatable
element type, but it has no explicit type hint, so it violates the type-hint
requirement.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f0efa89261543ae930766cfb6a3f995&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=2f0efa89261543ae930766cfb6a3f995&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/tasks/version_history_retention.py
**Line:** 358:358
**Comment:**
*Custom Rule: Add a type annotation to this merged table list variable
to comply with required type hints on relevant locals.
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=25d91c6d1cc768c30e9c95c248a573bfa61bbca6648bcb6fd5cdc22b8f22bdc5&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=25d91c6d1cc768c30e9c95c248a573bfa61bbca6648bcb6fd5cdc22b8f22bdc5&reaction=dislike'>π</a>
##########
superset/tasks/version_history_retention.py:
##########
@@ -0,0 +1,552 @@
+# 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.
+"""Celery task: prune old entity-version history.
+
+Retention is time-based. The task deletes parent + child shadow rows
+owned by ``version_transaction`` rows whose ``issued_at`` is older
+than ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` (default 30, env
+overridable, ``0`` to disable).
+
+One preservation rule, applied across every shadow table (parent,
+child, and the M2M association):
+
+* **Live** (``end_transaction_id IS NULL``) β never pruned.
+
+Baseline rows (``operation_type = 0``) and any closed historical row
+are subject to the same retention window as everything else. An
+entity that hasn't been edited within the window has only its live
+row remaining; the historical chain (including the synthetic
+baseline) ages out.
+
+If any shadow row anchored at a transaction is live (in a parent,
+child, or M2M shadow), the whole transaction is preserved (along with
+its other shadow rows and ``version_changes`` rows). Otherwise, all of
+the transaction's shadow rows are deleted and the ``version_transaction``
+row itself is dropped β its ``version_changes`` rows cascade via the FK.
+
+Registered via ``CeleryConfig.beat_schedule`` in ``superset/config.py``.
+Idempotent: a second run prunes nothing.
+"""
+
+from __future__ import annotations
+
+import logging
+import time
+from collections.abc import Iterator
+from dataclasses import dataclass
+from datetime import datetime, timedelta, timezone
+from typing import Any
+
+import sqlalchemy as sa
+from flask import current_app
+from sqlalchemy.exc import OperationalError
+
+from superset.extensions import celery_app, db, stats_logger_manager
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class ShadowTables:
+ """The four Continuum-managed Table objects the prune walks.
+
+ Bundled here so the prune helper's signature stays at two arguments
+ instead of five. The shape is set once at task entry by
+ ``_resolve_shadow_tables`` and threaded through the retry loop.
+ """
+
+ parent: list[sa.Table]
+ child: list[sa.Table]
+ m2m: sa.Table | None
+ transaction: sa.Table
+
+
+def _resolve_shadow_tables(tx_table: sa.Table) -> ShadowTables:
+ """Resolve the parent / child / m2m shadow Tables from Continuum's
+ mapper registry and bundle them with the transaction Table.
+
+ ``dashboard_slices_version`` is M2M-tracked by Continuum and lives
+ in metadata under that name (Continuum auto-creates the Table; it
+ isn't registered as a versioned class). Carried separately on the
+ ``ShadowTables`` dataclass because it doesn't follow the parent /
+ child class shape.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import version_class
+ from sqlalchemy_continuum.exc import ClassNotVersioned
+
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric,
TableColumn
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ # ``ClassNotVersioned`` is the only expected failure here β versioning
+ # init runs at startup; if it didn't, every class lookup raises this.
+ # Narrowing the catch keeps a real underlying failure (e.g. a metadata
+ # inconsistency after ``make_versioned``) from being silently swallowed
+ # into a no-op retention pass.
+ parent_tables: list[sa.Table] = []
+ for cls in (Dashboard, Slice, SqlaTable):
+ try:
+ parent_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ child_tables: list[sa.Table] = []
+ for cls in (TableColumn, SqlMetric):
+ try:
+ child_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ metadata = parent_tables[0].metadata if parent_tables else None
+ m2m_table = (
+ metadata.tables.get("dashboard_slices_version")
+ if metadata is not None
+ else None
+ )
+
+ return ShadowTables(
+ parent=parent_tables,
+ child=child_tables,
+ m2m=m2m_table,
+ transaction=tx_table,
+ )
+
+
+@dataclass(frozen=True)
+class _PruneWindow:
+ """One id-ordered window of prune candidates.
+
+ ``prunable`` is the subset of the window's candidate transactions
+ that are safe to delete. ``candidate_count`` and ``max_candidate_id``
+ drive the batch loop in :func:`_prune_old_versions_impl`: the loop
+ stops when a window returns fewer than ``_MAX_PRUNE_BATCH``
+ candidates, and otherwise advances ``after_id`` to
+ ``max_candidate_id`` to fetch the next window.
+ """
+
+ prunable: list[int]
+ candidate_count: int
+ max_candidate_id: int
+
+
+def _resolve_prune_window(
+ conn: sa.engine.Connection,
+ cutoff: datetime,
+ shadow_tables: list[sa.Table],
+ after_id: int,
+ batch_size: int,
+) -> _PruneWindow:
+ """Resolve one id-ordered window of ``version_transaction`` rows
+ eligible to prune: ``issued_at < cutoff`` AND ``id > after_id``,
+ ordered by ``id`` and capped at *batch_size*. From that window,
+ ``prunable`` excludes any transaction still serving as the live row
+ (``end_transaction_id IS NULL``) of any versioned entity in *any*
+ shadow table β parent, child, or M2M.
+
+ A child (``table_columns_version`` / ``sql_metrics_version``) or the
+ M2M association (``dashboard_slices_version``) is versioned on a
+ validity lifecycle independent of its parent: after a parent-only
+ edit, an unchanged child stays live anchored at an *older*
+ transaction than the parent's current live row. That older
+ transaction must be preserved too β otherwise
+ :func:`_delete_for_transactions` would drop the still-live
+ child/association row and silently strip the surviving version of its
+ columns / metrics / slices. Scanning every shadow for live rows (not
+ just parents) is what prevents that corruption.
+
+ Windowing by ``id`` (rather than materializing the whole backlog)
+ keeps per-pass memory and lock/transaction-hold time bounded. Live
+ rows older than the cutoff are never deleted but are skipped via the
+ ``after_id`` watermark, so the batch loop still terminates.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ # ``ORDER BY id LIMIT`` drives this off the primary key (clustered on
+ # InnoDB, rowid on SQLite, PK index on Postgres): it satisfies the
+ # ``id > after_id`` pagination and the ordering in one structure, with
+ # early termination and no sort. ``issued_at`` is applied as an inline
+ # filter. Because ids are monotonic with ``issued_at`` (append-only
+ # transaction log), the rows to prune cluster at the low-id end, so the
+ # PK scan is already efficient. A standalone ``issued_at`` index would
+ # force a filesort to satisfy ``ORDER BY id`` and so would not be chosen
+ # by the planner for this query on any supported engine β hence none is
+ # created.
+ candidate_ids = [
+ row[0]
+ for row in conn.execute(
+ sa.select(tx_table.c.id)
+ .where(tx_table.c.issued_at < cutoff)
+ .where(tx_table.c.id > after_id)
+ .order_by(tx_table.c.id)
+ .limit(batch_size)
+ )
+ ]
Review Comment:
**Suggestion:** Add an explicit type annotation for this list of transaction
IDs so the variableβs intended shape is enforced. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This local variable is a list of transaction IDs inferred from the query
result and can be explicitly annotated, so its lack of a type hint is a real
violation of the type-hint rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a3e21402e1a4150ab4471d8956dfefe&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=1a3e21402e1a4150ab4471d8956dfefe&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/tasks/version_history_retention.py
**Line:** 195:204
**Comment:**
*Custom Rule: Add an explicit type annotation for this list of
transaction IDs so the variableβs intended shape is enforced.
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=e930bc42e0be8e8f8b26b1f69a0d20ab30e2d30351a51c055fbabc5dbe3e6c73&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=e930bc42e0be8e8f8b26b1f69a0d20ab30e2d30351a51c055fbabc5dbe3e6c73&reaction=dislike'>π</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,137 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` β the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(self,
mock_logger):
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger
+ ):
+ """When the default ``CeleryConfig`` (or any class with the
+ entry) is in play, no warning fires. The happy path."""
+
+ class _CompleteCeleryConfig:
+ beat_schedule = {
+ "version_history.prune_old_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_CompleteCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_retention_task_registered_under_other_key(self,
mock_logger):
Review Comment:
**Suggestion:** Annotate this test methodβs parameter and return type to
satisfy the typing requirement. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This added test method lacks type annotations on its parameters and return
value, matching the type-hint violation described by the rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f236a251d334e91998944ecb5905850&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=4f236a251d334e91998944ecb5905850&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:** tests/unit_tests/initialization_test.py
**Line:** 584:584
**Comment:**
*Custom Rule: Annotate this test methodβs parameter and return type to
satisfy the typing requirement.
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=546c6a848ca3a26615ef710fd87d8e39e7c8b2a2d01c5374247b361e58296751&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=546c6a848ca3a26615ef710fd87d8e39e7c8b2a2d01c5374247b361e58296751&reaction=dislike'>π</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,137 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` β the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(self,
mock_logger):
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger
+ ):
+ """When the default ``CeleryConfig`` (or any class with the
+ entry) is in play, no warning fires. The happy path."""
+
+ class _CompleteCeleryConfig:
+ beat_schedule = {
+ "version_history.prune_old_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_CompleteCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_retention_task_registered_under_other_key(self,
mock_logger):
+ """The retention task registered under a non-matching schedule key
+ (a valid Celery config) MUST NOT warn: the check matches on each
+ entry's ``task``, not the schedule key."""
+
+ class _RenamedKeyCeleryConfig:
+ beat_schedule = {
+ "prune_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_RenamedKeyCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_config_is_none(self, mock_logger):
Review Comment:
**Suggestion:** Provide type annotations for this method argument and an
explicit return annotation. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The method is unannotated despite being new Python code that can clearly
accept type hints for its parameters and return type.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=425e75a6c3414472bb645896c09a6259&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=425e75a6c3414472bb645896c09a6259&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:** tests/unit_tests/initialization_test.py
**Line:** 602:602
**Comment:**
*Custom Rule: Provide type annotations for this method argument and an
explicit return annotation.
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=fd9e85b572ebfd844a00b88ba42de39f06456a5af32e04c76f85de4e6cf80f68&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=fd9e85b572ebfd844a00b88ba42de39f06456a5af32e04c76f85de4e6cf80f68&reaction=dislike'>π</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,137 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` β the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(self,
mock_logger):
Review Comment:
**Suggestion:** Add explicit type annotations for the test method parameters
and return type. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This new test method has no parameter or return type annotations, which
violates the rule requiring type hints on modified Python functions and methods
when they can be annotated.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea8327bc758344fe9f0fc3799bf1d4dc&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=ea8327bc758344fe9f0fc3799bf1d4dc&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:** tests/unit_tests/initialization_test.py
**Line:** 544:544
**Comment:**
*Custom Rule: Add explicit type annotations for the test method
parameters and return type.
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=fcaa825b4cf3424a3ed241a90ecab3da861cfe825f09dad89e407e16867b7a88&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=fcaa825b4cf3424a3ed241a90ecab3da861cfe825f09dad89e407e16867b7a88&reaction=dislike'>π</a>
##########
tests/integration_tests/versioning/retention_prune_tests.py:
##########
@@ -0,0 +1,351 @@
+# 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.
+"""Integration coverage for version-history retention pruning.
+
+Exercises ``superset.tasks.version_history_retention`` end-to-end against
+a real database: that an aged-out transaction's shadow rows are pruned
+while the live row is always preserved, and that the SERIALIZABLE pass
+retries on transient serialization failures and gives up after the cap.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+import pytest
+from sqlalchemy_continuum import version_class
+
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa:
F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _get_version_rows(dashboard: Dashboard) -> list[Any]:
+ ver_cls = version_class(Dashboard)
+ return (
+ db.session.query(ver_cls)
+ .filter(ver_cls.id == dashboard.id)
+ .order_by(ver_cls.transaction_id.asc())
+ .all()
+ )
+
+
+def _persist_fixture_state() -> None:
+ """Force the fixture's pending INSERTs to commit in their own transaction.
+
+ The birth_names fixture stages charts and the dashboard via session.add()
+ but does not commit. Without this, the test's first commit batches the
+ INSERTs and UPDATEs into the same Continuum transaction, causing the
+ existing version row to be updated in place instead of a new one being
+ created.
+ """
+ db.session.commit()
+
+
+class TestDashboardVersionRetention(SupersetTestCase):
+ """Retention pruning drops shadow rows older than
+ ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` while preserving live rows."""
+
+ @pytest.fixture(autouse=True)
+ def _load_data(self, load_birth_names_dashboard_with_slices): # noqa:
PT004, F811
Review Comment:
**Suggestion:** Add missing type hints to this fixture method by annotating
the fixture argument and declaring an explicit `None` return type. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The fixture method omits type hints for its parameter and does not declare
an explicit `-> None` return type. This is a real violation of the Python
type-hint requirement for new or modified code.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5a78b9af0fe6497c9261018d6fbf3bde&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=5a78b9af0fe6497c9261018d6fbf3bde&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:** tests/integration_tests/versioning/retention_prune_tests.py
**Line:** 67:68
**Comment:**
*Custom Rule: Add missing type hints to this fixture method by
annotating the fixture argument and declaring an explicit `None` return type.
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=51a4818970eef6ce10deb50242d40786ee1af3130ddbb72c87e5aac3cbdc02ba&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=51a4818970eef6ce10deb50242d40786ee1af3130ddbb72c87e5aac3cbdc02ba&reaction=dislike'>π</a>
##########
superset/tasks/version_history_retention.py:
##########
@@ -0,0 +1,552 @@
+# 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.
+"""Celery task: prune old entity-version history.
+
+Retention is time-based. The task deletes parent + child shadow rows
+owned by ``version_transaction`` rows whose ``issued_at`` is older
+than ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` (default 30, env
+overridable, ``0`` to disable).
+
+One preservation rule, applied across every shadow table (parent,
+child, and the M2M association):
+
+* **Live** (``end_transaction_id IS NULL``) β never pruned.
+
+Baseline rows (``operation_type = 0``) and any closed historical row
+are subject to the same retention window as everything else. An
+entity that hasn't been edited within the window has only its live
+row remaining; the historical chain (including the synthetic
+baseline) ages out.
+
+If any shadow row anchored at a transaction is live (in a parent,
+child, or M2M shadow), the whole transaction is preserved (along with
+its other shadow rows and ``version_changes`` rows). Otherwise, all of
+the transaction's shadow rows are deleted and the ``version_transaction``
+row itself is dropped β its ``version_changes`` rows cascade via the FK.
+
+Registered via ``CeleryConfig.beat_schedule`` in ``superset/config.py``.
+Idempotent: a second run prunes nothing.
+"""
+
+from __future__ import annotations
+
+import logging
+import time
+from collections.abc import Iterator
+from dataclasses import dataclass
+from datetime import datetime, timedelta, timezone
+from typing import Any
+
+import sqlalchemy as sa
+from flask import current_app
+from sqlalchemy.exc import OperationalError
+
+from superset.extensions import celery_app, db, stats_logger_manager
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class ShadowTables:
+ """The four Continuum-managed Table objects the prune walks.
+
+ Bundled here so the prune helper's signature stays at two arguments
+ instead of five. The shape is set once at task entry by
+ ``_resolve_shadow_tables`` and threaded through the retry loop.
+ """
+
+ parent: list[sa.Table]
+ child: list[sa.Table]
+ m2m: sa.Table | None
+ transaction: sa.Table
+
+
+def _resolve_shadow_tables(tx_table: sa.Table) -> ShadowTables:
+ """Resolve the parent / child / m2m shadow Tables from Continuum's
+ mapper registry and bundle them with the transaction Table.
+
+ ``dashboard_slices_version`` is M2M-tracked by Continuum and lives
+ in metadata under that name (Continuum auto-creates the Table; it
+ isn't registered as a versioned class). Carried separately on the
+ ``ShadowTables`` dataclass because it doesn't follow the parent /
+ child class shape.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import version_class
+ from sqlalchemy_continuum.exc import ClassNotVersioned
+
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric,
TableColumn
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ # ``ClassNotVersioned`` is the only expected failure here β versioning
+ # init runs at startup; if it didn't, every class lookup raises this.
+ # Narrowing the catch keeps a real underlying failure (e.g. a metadata
+ # inconsistency after ``make_versioned``) from being silently swallowed
+ # into a no-op retention pass.
+ parent_tables: list[sa.Table] = []
+ for cls in (Dashboard, Slice, SqlaTable):
+ try:
+ parent_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ child_tables: list[sa.Table] = []
+ for cls in (TableColumn, SqlMetric):
+ try:
+ child_tables.append(version_class(cls).__table__)
+ except ClassNotVersioned:
+ logger.warning(
+ "retention: %s is not versioned; skipping shadow", cls.__name__
+ )
+
+ metadata = parent_tables[0].metadata if parent_tables else None
+ m2m_table = (
+ metadata.tables.get("dashboard_slices_version")
+ if metadata is not None
+ else None
+ )
+
+ return ShadowTables(
+ parent=parent_tables,
+ child=child_tables,
+ m2m=m2m_table,
+ transaction=tx_table,
+ )
+
+
+@dataclass(frozen=True)
+class _PruneWindow:
+ """One id-ordered window of prune candidates.
+
+ ``prunable`` is the subset of the window's candidate transactions
+ that are safe to delete. ``candidate_count`` and ``max_candidate_id``
+ drive the batch loop in :func:`_prune_old_versions_impl`: the loop
+ stops when a window returns fewer than ``_MAX_PRUNE_BATCH``
+ candidates, and otherwise advances ``after_id`` to
+ ``max_candidate_id`` to fetch the next window.
+ """
+
+ prunable: list[int]
+ candidate_count: int
+ max_candidate_id: int
+
+
+def _resolve_prune_window(
+ conn: sa.engine.Connection,
+ cutoff: datetime,
+ shadow_tables: list[sa.Table],
+ after_id: int,
+ batch_size: int,
+) -> _PruneWindow:
+ """Resolve one id-ordered window of ``version_transaction`` rows
+ eligible to prune: ``issued_at < cutoff`` AND ``id > after_id``,
+ ordered by ``id`` and capped at *batch_size*. From that window,
+ ``prunable`` excludes any transaction still serving as the live row
+ (``end_transaction_id IS NULL``) of any versioned entity in *any*
+ shadow table β parent, child, or M2M.
+
+ A child (``table_columns_version`` / ``sql_metrics_version``) or the
+ M2M association (``dashboard_slices_version``) is versioned on a
+ validity lifecycle independent of its parent: after a parent-only
+ edit, an unchanged child stays live anchored at an *older*
+ transaction than the parent's current live row. That older
+ transaction must be preserved too β otherwise
+ :func:`_delete_for_transactions` would drop the still-live
+ child/association row and silently strip the surviving version of its
+ columns / metrics / slices. Scanning every shadow for live rows (not
+ just parents) is what prevents that corruption.
+
+ Windowing by ``id`` (rather than materializing the whole backlog)
+ keeps per-pass memory and lock/transaction-hold time bounded. Live
+ rows older than the cutoff are never deleted but are skipped via the
+ ``after_id`` watermark, so the batch loop still terminates.
+ """
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ # ``ORDER BY id LIMIT`` drives this off the primary key (clustered on
+ # InnoDB, rowid on SQLite, PK index on Postgres): it satisfies the
+ # ``id > after_id`` pagination and the ordering in one structure, with
+ # early termination and no sort. ``issued_at`` is applied as an inline
+ # filter. Because ids are monotonic with ``issued_at`` (append-only
+ # transaction log), the rows to prune cluster at the low-id end, so the
+ # PK scan is already efficient. A standalone ``issued_at`` index would
+ # force a filesort to satisfy ``ORDER BY id`` and so would not be chosen
+ # by the planner for this query on any supported engine β hence none is
+ # created.
+ candidate_ids = [
+ row[0]
+ for row in conn.execute(
+ sa.select(tx_table.c.id)
+ .where(tx_table.c.issued_at < cutoff)
+ .where(tx_table.c.id > after_id)
+ .order_by(tx_table.c.id)
+ .limit(batch_size)
+ )
+ ]
+ if not candidate_ids:
+ return _PruneWindow(prunable=[], candidate_count=0,
max_candidate_id=after_id)
+
+ # The select is ordered by ``id`` ascending, so the last element is
+ # the watermark for the next window.
+ max_candidate_id = candidate_ids[-1]
+
+ # Build the set of transaction ids that still anchor a live row
+ # (``end_transaction_id IS NULL``) in some shadow table. Those
+ # transactions represent the current state of an entity (or one of
+ # its children / associations) and must be preserved regardless of
+ # age. Chunked over the candidates to keep the bind-parameter count
+ # inside SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` floor (see
+ # ``_TX_ID_CHUNK_SIZE`` below). Ids already confirmed live by an
+ # earlier table are skipped before probing the next one.
+ preserved_ids: set[int] = set()
+ for stbl in shadow_tables:
+ remaining = [tx_id for tx_id in candidate_ids if tx_id not in
preserved_ids]
+ if not remaining:
+ break
+ for chunk in _chunked(remaining, _TX_ID_CHUNK_SIZE):
+ for row in conn.execute(
+ sa.select(stbl.c.transaction_id)
+ .where(stbl.c.transaction_id.in_(chunk))
+ .where(stbl.c.end_transaction_id.is_(None))
+ .distinct()
+ ):
+ preserved_ids.add(row[0])
+
+ prunable = [tx_id for tx_id in candidate_ids if tx_id not in preserved_ids]
+ return _PruneWindow(
+ prunable=prunable,
+ candidate_count=len(candidate_ids),
+ max_candidate_id=max_candidate_id,
+ )
+
+
+# SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` defaults to 999 (lifted to
+# 32766 in 3.32+ but the older limit can still apply in shipped
+# builds). Postgres + MySQL handle tens of thousands of bind params
+# without complaint, so the chunk size is dictated by the SQLite floor.
+# 500 keeps each single-column DELETE / SELECT (see
``_delete_for_transactions``,
+# which splits the create/close predicates into two statements) well inside
+# that floor, with margin for any other bound params in the surrounding
+# statement.
+_TX_ID_CHUNK_SIZE = 500
+
+# Maximum ``version_transaction`` rows resolved + pruned per SERIALIZABLE
+# pass. The prune loops over id-ordered windows of this size (see
+# ``_prune_old_versions_impl``) so memory and lock/transaction-hold time
+# stay bounded per pass instead of scaling with the full backlog on the
+# first run after deploy.
+_MAX_PRUNE_BATCH = 1000
+
+
+def _delete_for_transactions(
+ conn: sa.engine.Connection,
+ tables: list[sa.Table],
+ tx_ids: list[int],
+) -> int:
+ """Delete shadow rows in *tables* whose lifespan touches a pruned
+ transaction β either ``transaction_id`` (created at) or
+ ``end_transaction_id`` (closed at) is in *tx_ids*. Returns total
+ rowcount across all tables.
+
+ The ``end_transaction_id`` predicate is required to keep referential
+ integrity when transactions span multiple entities. A flush that
+ saves dashboard + slice + dataset at the same ``tx=X`` produces
+ three shadow rows sharing that tx. If only the dashboard is later
+ edited at ``tx=Y``, the dashboard row at ``tx=X`` is closed
+ (``end_tx=Y``) while the slice/dataset rows stay live at
+ ``tx=X``. Retention preserves ``tx=X`` (slice/dataset are live
+ there) and prunes ``tx=Y``. Without the ``end_tx`` predicate, the
+ dashboard's closed row at ``tx=X`` survives step 1 β its
+ ``end_transaction_id=Y`` then violates the FK when step 2 deletes
+ ``version_transaction`` row ``Y``.
+
+ Live rows are never matched by either predicate
+ (``end_transaction_id IS NULL`` is not ``IN`` anything; live rows'
+ ``transaction_id`` is preserved by construction in
+ :func:`_resolve_prune_window`).
+
+ ``tx_ids`` is chunked into batches of ``_TX_ID_CHUNK_SIZE`` so the
+ bind-parameter count stays inside SQLite's ``SQLITE_MAX_VARIABLE_
+ NUMBER`` limit. Postgres and MySQL would happily accept the full
+ list, but the floor is dialect-agnostic since the retention task is
+ the only path that accumulates open-ended id batches.
+ """
+ if not tx_ids:
+ return 0
+ total = 0
+ for tbl in tables:
+ for chunk in _chunked(tx_ids, _TX_ID_CHUNK_SIZE):
+ # Two single-column DELETEs rather than one ``OR`` of both
+ # columns. The OR form binds every id twice (once for
+ # ``transaction_id``, once for ``end_transaction_id``), so a
+ # full chunk would bind ``2 * _TX_ID_CHUNK_SIZE`` params and
+ # overflow SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` floor. Each
+ # single-column statement binds at most ``_TX_ID_CHUNK_SIZE``.
+ # A row whose create- and close-tx are both pruned is removed
+ # by the first matching DELETE; the second finds nothing, so
+ # there is no double count.
+ for col in (tbl.c.transaction_id, tbl.c.end_transaction_id):
+ result = conn.execute(sa.delete(tbl).where(col.in_(chunk)))
+ total += result.rowcount or 0
+ return total
+
+
+def _chunked(items: list[int], size: int) -> Iterator[list[int]]:
+ """Yield *items* in fixed-size lists. Final chunk may be smaller."""
+ for i in range(0, len(items), size):
+ yield items[i : i + size]
+
+
+#: Maximum number of attempts the prune will make before giving up.
+#: A daily Celery beat schedule means the next chance is 24h out, so
+#: a small inline retry materially improves the recovery time for the
+#: serialization-conflict path.
+_MAX_RETRY_ATTEMPTS = 3
+
+#: Base for exponential backoff between retries (seconds). Worst-case
+#: extra latency with the 3-attempt cap above and the factor below is
+#: ``BASE + BASE * FACTOR`` = ~0.5s β well inside the prune's own
+#: typical runtime.
+_RETRY_BACKOFF_BASE_SECONDS = 0.1
+
+#: Exponential-backoff multiplier between successive retry attempts.
+#: Backoff for attempt N is ``BASE * (FACTOR ** (N - 1))``.
+_RETRY_BACKOFF_FACTOR = 4
+
+#: Statsd metric prefix for retention emissions. Mirrors the activity-view
+#: orchestrator's ``superset.activity_view.*`` namespace so a single
+#: Grafana filter (``superset.versioning.*``) catches both sides of the
+#: feature. The pruned-count gauge fires every run; the skipped counter
+#: fires for the "retention disabled" and "no versioned classes" cases;
+#: the retried counter fires when the SERIALIZABLE block tripped at
+#: least one conflict before settling.
+_METRIC_PREFIX = "superset.versioning.retention"
+
+
+def _run_prune_pass(
+ cutoff: datetime, tables: ShadowTables, after_id: int = 0
+) -> dict[str, Any]:
+ """One SERIALIZABLE pass over a single id-ordered window of candidate
+ transactions starting after ``after_id``. The caller wraps this in
+ the retry loop (serialization conflict β fresh connection from a
+ clean snapshot) and the batch loop (advance ``after_id`` until the
+ backlog drains). The returned ``candidate_count`` / ``max_candidate_id``
+ drive that batch loop."""
+ # Scan every shadow (parent + child + M2M) for live rows, not just
+ # parents: children and the M2M association live on independent
+ # validity lifecycles and may anchor a still-live row at an older
+ # transaction than the parent's current live row.
+ live_bearing_tables = [*tables.parent, *tables.child]
+ if tables.m2m is not None:
+ live_bearing_tables.append(tables.m2m)
+
+ # The Celery task runs outside the request-bound DB session, so we
+ # use a fresh connection rather than ``db.session`` to avoid stepping
+ # on web-request state.
+ with (
+ db.engine.connect().execution_options(isolation_level="SERIALIZABLE")
as conn,
+ conn.begin(),
+ ):
+ window = _resolve_prune_window(
+ conn, cutoff, live_bearing_tables, after_id, _MAX_PRUNE_BATCH
+ )
+ tx_ids = window.prunable
+
+ parent_rows = _delete_for_transactions(conn, tables.parent, tx_ids)
+ child_rows = _delete_for_transactions(conn, tables.child, tx_ids)
+ m2m_rows = (
+ _delete_for_transactions(conn, [tables.m2m], tx_ids)
+ if tables.m2m is not None
+ else 0
+ )
+
+ # Drop the version_transaction rows themselves. ON DELETE
+ # CASCADE on version_changes.transaction_id removes the
+ # associated change records automatically. Same SQLite bind-
+ # parameter chunking applies as the shadow deletes above.
+ tx_rows = 0
+ for chunk in _chunked(tx_ids, _TX_ID_CHUNK_SIZE):
+ tx_rows += (
+ conn.execute(
+ sa.delete(tables.transaction).where(
+ tables.transaction.c.id.in_(chunk)
+ )
+ ).rowcount
+ or 0
+ )
+
+ return {
+ "cutoff": cutoff.isoformat(),
+ "candidate_count": window.candidate_count,
+ "max_candidate_id": window.max_candidate_id,
+ "pruned_transactions": tx_rows,
+ "pruned_parent_shadows": parent_rows,
+ "pruned_child_shadows": child_rows,
+ "pruned_m2m_shadows": m2m_rows,
+ }
+
+
+def _run_pass_with_retry(
+ cutoff: datetime, tables: ShadowTables, after_id: int
+) -> tuple[dict[str, Any], int]:
+ """Run one window pass, retrying on serialization conflict. Returns
+ ``(stats, retries_used)``; re-raises the ``OperationalError`` if all
+ ``_MAX_RETRY_ATTEMPTS`` attempts conflict.
+
+ Postgres surfaces conflicts as ``SerializationFailure`` (a subclass
+ of ``sqlalchemy.exc.OperationalError``). The catch is deliberately the
+ broader ``OperationalError`` β it also covers transient faults such as
+ SQLite's "database is locked" and dropped connections, all of which are
+ safe to retry because each pass is idempotent and runs in its own fresh
+ transaction. Without the inline retry a single conflict pushes the next
+ attempt 24h out (daily Celery beat), so under sustained write pressure
+ the prune could silently fail for days in a row.
+ """
+ for attempt in range(1, _MAX_RETRY_ATTEMPTS + 1):
+ try:
+ return _run_prune_pass(cutoff, tables, after_id), attempt - 1
+ except OperationalError as exc:
+ stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.retried")
+ if attempt == _MAX_RETRY_ATTEMPTS:
+ logger.warning(
+ "version_history_retention: gave up after %d attempts: %s",
+ _MAX_RETRY_ATTEMPTS,
+ exc,
+ )
+ raise
+ backoff = _RETRY_BACKOFF_BASE_SECONDS * (
+ _RETRY_BACKOFF_FACTOR ** (attempt - 1)
+ )
+ logger.info(
+ "version_history_retention: attempt %d hit %s; retrying in
%.2fs",
+ attempt,
+ type(exc).__name__,
+ backoff,
+ )
+ time.sleep(backoff)
+ raise AssertionError("retention retry loop exited without result")
+
+
+def _prune_old_versions_impl(retention_days: int) -> dict[str, Any]:
+ """Pure-Python implementation of the prune. Split out from the
+ Celery task wrapper so unit tests can call it directly without the
+ Celery harness.
+
+ Returns a stats dict for logging / test assertions.
+
+ Isolation level: SERIALIZABLE. The prune is logically a multi-step
+ read-then-write (candidate-vs-preserved SELECTs feeding the shadow
+ DELETEs). At READ COMMITTED there is a TOCTOU window β a save
+ committing between the preserved-ids snapshot and the DELETEs can
+ leave a stale view of which transaction ids are still serving as
+ the live row of some entity, and a shadow row that became live
+ mid-task can be silently dropped. SERIALIZABLE makes the prune
+ atomic against concurrent writers. SQLite is single-writer so
+ SERIALIZABLE is the only level available; MySQL InnoDB and Postgres
+ both support it natively.
+
+ Postgres surfaces conflicts as ``SerializationFailure`` (a subclass
+ of ``sqlalchemy.exc.OperationalError``). The prune retries up to
+ ``_MAX_RETRY_ATTEMPTS`` with exponential backoff before giving up
+ and letting the outer Celery wrapper log + return ``{"error": 1}``.
+ Without the inline retry, a single conflict pushes the next attempt
+ 24 hours out (daily Celery beat), and under sustained write
+ pressure the prune can silently fail for many days in a row.
+ """
+ if retention_days <= 0:
+ logger.info(
+ "version_history_retention:
SUPERSET_VERSION_HISTORY_RETENTION_DAYS "
+ "<= 0; skipping",
+ )
+ stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.skipped")
+ return {"skipped": 1}
+
+ # pylint: disable=import-outside-toplevel
+ from sqlalchemy_continuum import versioning_manager
+
+ tables =
_resolve_shadow_tables(versioning_manager.transaction_cls.__table__)
+ if not tables.parent:
+ logger.warning(
+ "version_history_retention: no versioned classes resolved;
skipping",
+ )
+ stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.skipped")
+ return {"skipped": 1}
+
+ # Naive-UTC to match ``version_transaction.issued_at`` (Continuum stores
+ # it tz-naive via ``utc_now()``); ``datetime.utcnow()`` is deprecated on
+ # 3.12+, so derive the same value from the tz-aware clock and drop tzinfo.
+ cutoff = datetime.now(timezone.utc).replace(tzinfo=None) - timedelta(
+ days=retention_days
+ )
+
+ # Drain the backlog one bounded, id-ordered window at a time. Each
+ # window is its own retried SERIALIZABLE pass, so memory and
+ # lock/transaction-hold time stay bounded per pass even on the first
+ # run after deploy. The loop stops once a window returns fewer than a
+ # full batch of candidates (nothing left older than the cutoff).
+ totals = {
+ "pruned_transactions": 0,
+ "pruned_parent_shadows": 0,
+ "pruned_child_shadows": 0,
+ "pruned_m2m_shadows": 0,
+ }
Review Comment:
**Suggestion:** Add an explicit type annotation to this accumulator
dictionary to satisfy the type-hint requirement for relevant local variables.
[custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This new local variable is a relevant accumulator dictionary and is not
type-annotated, so it matches the Python type-hint rule for annotatable
variables.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=38c10fdf125d45648966c2c42b9d00e0&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=38c10fdf125d45648966c2c42b9d00e0&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/tasks/version_history_retention.py
**Line:** 507:511
**Comment:**
*Custom Rule: Add an explicit type annotation to this accumulator
dictionary to satisfy the type-hint requirement for relevant local 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=75e51558a103c5c0876b324a32bf84e4e996cd85f174dea93abcc99a9ca83737&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=75e51558a103c5c0876b324a32bf84e4e996cd85f174dea93abcc99a9ca83737&reaction=dislike'>π</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,137 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` β the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(self,
mock_logger):
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger
+ ):
Review Comment:
**Suggestion:** Add explicit type annotations for both parameters and the
return type in this test method signature. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The method signature omits type annotations for both parameters and the
return type, so it does violate the Python type-hint requirement.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=27570cc7b9484332bb5372d38f82df83&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=27570cc7b9484332bb5372d38f82df83&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:** tests/unit_tests/initialization_test.py
**Line:** 565:567
**Comment:**
*Custom Rule: Add explicit type annotations for both parameters and the
return type in this test method signature.
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=2385d9e632d5d8400e81971ef5ddb14a5396d31da1d0b40c1b71d2a73dbc17ab&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=2385d9e632d5d8400e81971ef5ddb14a5396d31da1d0b40c1b71d2a73dbc17ab&reaction=dislike'>π</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,137 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` β the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(self,
mock_logger):
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger
+ ):
+ """When the default ``CeleryConfig`` (or any class with the
+ entry) is in play, no warning fires. The happy path."""
+
+ class _CompleteCeleryConfig:
+ beat_schedule = {
+ "version_history.prune_old_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_CompleteCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_retention_task_registered_under_other_key(self,
mock_logger):
+ """The retention task registered under a non-matching schedule key
+ (a valid Celery config) MUST NOT warn: the check matches on each
+ entry's ``task``, not the schedule key."""
+
+ class _RenamedKeyCeleryConfig:
+ beat_schedule = {
+ "prune_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_RenamedKeyCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_config_is_none(self, mock_logger):
+ """``CELERY_CONFIG = None`` is the documented "disable Celery
+ entirely" path. The warn-log MUST NOT fire β the operator made
+ a deliberate choice; complaining about a missing retention entry
+ on a Celery-disabled deployment trains operators to ignore the
+ warning."""
+ initializer = self._initializer({"CELERY_CONFIG": None})
+ initializer._warn_if_retention_beat_missing()
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_dict_form_celery_config_with_entry_does_not_warn(self,
mock_logger):
Review Comment:
**Suggestion:** Add type hints for the mocked logger parameter and return
type in this newly added test method. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This test method does not include type annotations for its parameters or
return type, which is exactly the kind of omission the rule flags.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=98e4061a0ad94aa3ae40a6fa2f5d0ef0&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=98e4061a0ad94aa3ae40a6fa2f5d0ef0&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:** tests/unit_tests/initialization_test.py
**Line:** 613:613
**Comment:**
*Custom Rule: Add type hints for the mocked logger parameter and return
type in this newly added test method.
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=ea68e63659ba3ce8a532f18d0938591e8ab675c0856ce3f49c501d2d6c8782e8&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=ea68e63659ba3ce8a532f18d0938591e8ab675c0856ce3f49c501d2d6c8782e8&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]
