codeant-ai-for-open-source[bot] commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3537952225
##########
superset/dashboards/api.py:
##########
@@ -850,17 +908,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 explicit type annotations for the version-info local
variables to satisfy the type-hint requirement for relevant new variables.
[custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
The new local variables `old_info` and `new_info` are assigned values from
`current_entity_version_info(...)`, whose return type is a concrete
`EntityVersionInfo` dataclass. Under the type-hint rule, these newly introduced
variables can and should be annotated explicitly, so this is a real violation.
</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=38f9f02379f14df189140779887fbc18&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=38f9f02379f14df189140779887fbc18&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:** 914:922
**Comment:**
*Custom Rule: Add explicit type annotations for the version-info local
variables to satisfy the type-hint requirement for relevant new 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%2F41176&comment_hash=16b7d948a0eee63c8bea826145b527f9efca5d75986437e43679f2ddccb58095&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=16b7d948a0eee63c8bea826145b527f9efca5d75986437e43679f2ddccb58095&reaction=dislike'>π</a>
##########
superset/datasets/api.py:
##########
@@ -464,17 +527,69 @@ 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(SqlaTable, pk)
+
try:
+ # Two commands, two commits, two Continuum transactions for an
+ # ``override_columns`` save β deliberately NOT merged into one
+ # transaction. A single-transaction design was attempted and
+ # reverted: ``DBEventLogger`` writes request logs through the
+ # SHARED scoped session and calls ``commit()`` /
+ # ``rollback()`` on it mid-request (superset/utils/log.py),
+ # so any save held uncommitted across a logged sub-action can
+ # be committed half-done (Postgres/MySQL) or rolled back
+ # entirely on a transient logger failure (SQLite's
+ # "database is locked"). Until the event logger gets its own
+ # session, per-command commit boundaries are the only shape
+ # whose failure modes are honest. Consequence the
+ # version-history UI must tolerate: one logical save can
+ # surface as two version transactions stamped the same second.
changed_model = UpdateDatasetCommand(pk, item,
override_columns).run()
+ # Capture the post-update identifiers BEFORE the refresh:
+ # RefreshDatasetCommand commits its own transaction, so reading
+ # afterwards would attribute the refresh's version to the
+ # user's update (and oldβnew would span two transactions).
+ new_info = current_entity_version_info(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this post-update version
info variable (for example, `EntityVersionInfo`). [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This modified code introduces a local variable without any type annotation,
and it is a relevant variable that can be annotated under 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=b9e607c4a8604cae9a0a12ff2a9cee26&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=b9e607c4a8604cae9a0a12ff2a9cee26&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/datasets/api.py
**Line:** 555:557
**Comment:**
*Custom Rule: Add an explicit type annotation for this post-update
version info variable (for example, `EntityVersionInfo`).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=e9f6ecd7c32cb736134d6fd0356cc46518c6a8bf21153fb09769817d82a94f7b&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=e9f6ecd7c32cb736134d6fd0356cc46518c6a8bf21153fb09769817d82a94f7b&reaction=dislike'>π</a>
##########
superset/datasets/api.py:
##########
@@ -464,17 +527,69 @@ 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(SqlaTable, pk)
+
try:
+ # Two commands, two commits, two Continuum transactions for an
+ # ``override_columns`` save β deliberately NOT merged into one
+ # transaction. A single-transaction design was attempted and
+ # reverted: ``DBEventLogger`` writes request logs through the
+ # SHARED scoped session and calls ``commit()`` /
+ # ``rollback()`` on it mid-request (superset/utils/log.py),
+ # so any save held uncommitted across a logged sub-action can
+ # be committed half-done (Postgres/MySQL) or rolled back
+ # entirely on a transient logger failure (SQLite's
+ # "database is locked"). Until the event logger gets its own
+ # session, per-command commit boundaries are the only shape
+ # whose failure modes are honest. Consequence the
+ # version-history UI must tolerate: one logical save can
+ # surface as two version transactions stamped the same second.
changed_model = UpdateDatasetCommand(pk, item,
override_columns).run()
+ # Capture the post-update identifiers BEFORE the refresh:
+ # RefreshDatasetCommand commits its own transaction, so reading
+ # afterwards would attribute the refresh's version to the
+ # user's update (and oldβnew would span two transactions).
+ new_info = current_entity_version_info(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ etag_version_uuid = new_info.version_uuid
Review Comment:
**Suggestion:** Add an explicit nullable string type annotation when
introducing this ETag version UUID variable (for example, `str | None`).
[custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This is another newly introduced local variable in modified Python code with
no type hint, so the type-hints rule applies.
</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=dcef8a9f8903444aa6a589f1c611e82d&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=dcef8a9f8903444aa6a589f1c611e82d&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/datasets/api.py
**Line:** 558:558
**Comment:**
*Custom Rule: Add an explicit nullable string type annotation when
introducing this ETag version UUID variable (for example, `str | None`).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=aceeb125575289eefebafea8f80697625f1717a51732b7cb2118b92f14b828d1&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=aceeb125575289eefebafea8f80697625f1717a51732b7cb2118b92f14b828d1&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 from
`superset.migrations.shared.utils` for this standard index creation path
instead of calling Alembic index APIs directly. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
The migration shared utils module already provides `create_index`, which
centralizes existence checks and Alembic compatibility handling. This code
calls Alembic's `op.create_index` directly instead of the shared helper, so the
suggestion matches a real violation of the migration-use-shared-utils rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.github/copilot-instructions.md (line 294)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=518bf5004e364e62b16493a341a8ca4c&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=518bf5004e364e62b16493a341a8ca4c&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 from
`superset.migrations.shared.utils` for this standard index creation path
instead of calling Alembic index APIs 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%2F41176&comment_hash=2a68fad3ec01192784751dac2e391cb2bec2e6009d5d99d05000226fdc8d71a6&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=2a68fad3ec01192784751dac2e391cb2bec2e6009d5d99d05000226fdc8d71a6&reaction=dislike'>π</a>
##########
superset/datasets/api.py:
##########
@@ -464,17 +527,69 @@ 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(SqlaTable, pk)
Review Comment:
**Suggestion:** Add an explicit type annotation for this version info
variable (for example, annotate it as `EntityVersionInfo`). [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This newly added local variable is not type-annotated even though it is a
clearly annotatable Python variable introduced in modified code, so it matches
the type-hints 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=825e8f1239894ad196581b271f733549&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=825e8f1239894ad196581b271f733549&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/datasets/api.py
**Line:** 533:533
**Comment:**
*Custom Rule: Add an explicit type annotation for this version info
variable (for example, annotate it as `EntityVersionInfo`).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=9041e2c709755feea4136e1b77b4b69e5ae64a25b6779d6d111e3592aad875ae&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=9041e2c709755feea4136e1b77b4b69e5ae64a25b6779d6d111e3592aad875ae&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]
