codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3560213950
##########
superset/config.py:
##########
@@ -1575,6 +1575,18 @@ def sync_theme_logo_href(
os.environ.get("ENABLE_VERSIONING_CAPTURE", "false")
)
+# Retention window (days) for entity version history. Version rows
+# whose owning ``version_transaction.issued_at`` is older than this
+# value are pruned by the ``version_history.prune_old_versions``
+# Celery beat task (registered below in ``CeleryConfig.beat_schedule``).
+# Only the live row (``end_transaction_id IS NULL``) is preserved
+# unconditionally; baseline rows (``operation_type=0``) and any
+# historical row age out alongside the rest. ``0`` disables pruning.
Review Comment:
**Suggestion:** The retention comment is inconsistent with actual task
behavior: the prune implementation preserves the entire transaction whenever
any anchored shadow row is live (not just that single live row), and it skips
pruning for any non-positive value (`<= 0`), not only `0`. Update this comment
to match the real contract so operators donโt configure retention based on
incorrect assumptions. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Operators misconfigure retention thinking only live rows preserved.
- โ ๏ธ Negative retention values disable pruning contrary to comment.
- โ ๏ธ Storage growth predictions inaccurate due to misunderstood retention
behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `superset/config.py` and locate the retention configuration comment
at lines
1578โ1585 in the PR hunk, which states: "Only the live row
(``end_transaction_id IS
NULL``) is preserved unconditionally; baseline rows (``operation_type=0``)
and any
historical row age out alongside the rest. ``0`` disables pruning."
2. Open `superset/tasks/version_history_retention.py` and read the module
docstring at
lines 17โ39, which explicitly documents that if any shadow row anchored at a
`version_transaction` is live (parent, child, or M2M), "the whole
transaction is preserved
(along with its other shadow rows and `version_changes` rows)."
3. In the same file `superset/tasks/version_history_retention.py`, inspect
`_resolve_prune_window()` around lines 158โ239: it builds `preserved_ids`
from any shadow
table where `end_transaction_id IS NULL`, then marks all such transaction
ids as
non-prunable, meaning the entire transaction and all its shadow rows are
preserved, not
just the single live row.
4. Still in `superset/tasks/version_history_retention.py`, inspect
`_prune_old_versions_impl()` at lines 76โ82, which short-circuits when
`retention_days <=
0`, logging `"SUPERSET_VERSION_HISTORY_RETENTION_DAYS <= 0; skipping"`,
showing that both
`0` and negative values fully disable pruning, whereas the comment in
`superset/config.py`
only mentions `0` as disabling and omits `< 0`, creating a mismatch between
documented and
actual behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec19554360d94a389088fd81a24ef78b&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=ec19554360d94a389088fd81a24ef78b&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/config.py
**Line:** 1582:1584
**Comment:**
*Comment Mismatch: The retention comment is inconsistent with actual
task behavior: the prune implementation preserves the entire transaction
whenever any anchored shadow row is live (not just that single live row), and
it skips pruning for any non-positive value (`<= 0`), not only `0`. Update this
comment to match the real contract so operators donโt configure retention based
on incorrect assumptions.
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=f998e3e0565090fa8c46c7e857e7c4472268dbb5cb0f44b2717d0cda1dc3428b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=f998e3e0565090fa8c46c7e857e7c4472268dbb5cb0f44b2717d0cda1dc3428b&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]
