codeant-ai-for-open-source[bot] commented on code in PR #41176: URL: https://github.com/apache/superset/pull/41176#discussion_r3500129226
########## scripts/seed_junction_load.py: ########## @@ -0,0 +1,679 @@ +#!/usr/bin/env python3 +# 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. +# +# ---------------------------------------------------------------------- +# Stress-test data generator for the composite-PK migration (sc-105349). +# +# Bulk-inserts synthetic parent rows and many-to-many junction rows for +# the eight association tables that the composite-PK migration touches. +# Useful for measuring migration runtime at varying scales β run this at +# 100K / 1M / 5M / 10M rows and time the migration at each scale to +# verify the O(N log N) extrapolation. +# +# Idempotent: rerunning with the same target is a no-op; rerunning with +# a higher target adds rows up to the new total. Batched bulk INSERTs +# (10K rows per statement) make it fast on Postgres, MySQL, and SQLite. +# +# Usage (inside the Superset container): +# +# docker exec superset-superset-1 \\ +# /app/.venv/bin/python /app/scripts/seed_junction_load.py \\ +# --dashboard-slices 1000000 \\ +# --slice-user 100000 \\ +# --dashboard-user 100000 +# +# Run with no flags for the defaults shown below. Use ``--dry-run`` to +# print the planned inserts without writing anything. +# +# The script connects via Superset's standard ``DATABASE_*`` env vars +# (or ``SUPERSET__SQLALCHEMY_DATABASE_URI`` if set), so it works +# automatically inside the Superset container regardless of which +# metadata DB backend is in use. + +from __future__ import annotations + +import argparse +import logging +import os +import sys +import time +from contextlib import contextmanager +from typing import Iterator +from uuid import uuid4 + +import sqlalchemy as sa +from sqlalchemy.engine import Connection, Engine + +logger = logging.getLogger("seed_junction_load") Review Comment: **Suggestion:** Add an explicit type annotation to this logger variable declaration. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> This module-level variable is not type-annotated even though its type is clear and annotatable, so it matches the custom rule requiring type hints on relevant variables. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f3f0e2db25654bbfade207fc7ba18da9&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=f3f0e2db25654bbfade207fc7ba18da9&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:** scripts/seed_junction_load.py **Line:** 62:62 **Comment:** *Custom Rule: Add an explicit type annotation to this logger variable declaration. 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=9abd681e9dcb139e0dda65d7952ab082fff9186f04dcfe434d495e1f8ea7486a&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=9abd681e9dcb139e0dda65d7952ab082fff9186f04dcfe434d495e1f8ea7486a&reaction=dislike'>π</a> ########## scripts/seed_junction_load.py: ########## @@ -0,0 +1,679 @@ +#!/usr/bin/env python3 +# 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. +# +# ---------------------------------------------------------------------- +# Stress-test data generator for the composite-PK migration (sc-105349). +# +# Bulk-inserts synthetic parent rows and many-to-many junction rows for +# the eight association tables that the composite-PK migration touches. +# Useful for measuring migration runtime at varying scales β run this at +# 100K / 1M / 5M / 10M rows and time the migration at each scale to +# verify the O(N log N) extrapolation. +# +# Idempotent: rerunning with the same target is a no-op; rerunning with +# a higher target adds rows up to the new total. Batched bulk INSERTs +# (10K rows per statement) make it fast on Postgres, MySQL, and SQLite. +# +# Usage (inside the Superset container): +# +# docker exec superset-superset-1 \\ +# /app/.venv/bin/python /app/scripts/seed_junction_load.py \\ +# --dashboard-slices 1000000 \\ +# --slice-user 100000 \\ +# --dashboard-user 100000 +# +# Run with no flags for the defaults shown below. Use ``--dry-run`` to +# print the planned inserts without writing anything. +# +# The script connects via Superset's standard ``DATABASE_*`` env vars +# (or ``SUPERSET__SQLALCHEMY_DATABASE_URI`` if set), so it works +# automatically inside the Superset container regardless of which +# metadata DB backend is in use. + +from __future__ import annotations + +import argparse +import logging +import os +import sys +import time +from contextlib import contextmanager +from typing import Iterator +from uuid import uuid4 + +import sqlalchemy as sa +from sqlalchemy.engine import Connection, Engine + +logger = logging.getLogger("seed_junction_load") + +# Bulk INSERT batch size. Larger values = fewer statements but more memory. +BATCH = 10_000 Review Comment: **Suggestion:** Add an explicit integer type annotation for this module-level constant. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> This module-level constant is left unannotated, and the rule explicitly flags new or modified Python code that omits type hints on variables that can be annotated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dfdd4ba2e6cf435899ddfe74697d7f09&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=dfdd4ba2e6cf435899ddfe74697d7f09&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:** scripts/seed_junction_load.py **Line:** 65:65 **Comment:** *Custom Rule: Add an explicit integer type annotation for this module-level constant. 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=70cc6baeff4ab6d7d2019866a650f4a17c2659448b559d53d5ae5eb022541ffb&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=70cc6baeff4ab6d7d2019866a650f4a17c2659448b559d53d5ae5eb022541ffb&reaction=dislike'>π</a> ########## superset/charts/api.py: ########## @@ -437,9 +495,29 @@ 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, so this stays inert under the + # kill-switch). + old_info = current_entity_version_info(Slice, pk) + try: changed_model = UpdateChartCommand(pk, item).run() - response = self.response(200, id=changed_model.id, result=item) + new_info = current_entity_version_info( Review Comment: **Suggestion:** Add explicit type annotations for the newly introduced version-info local variables to satisfy the type-hint requirement for relevant annotatable variables. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The custom rule requires type hints on relevant annotatable variables. These two newly introduced locals store structured version metadata and are not annotated, so the suggestion correctly identifies a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c5a538d9aa14367a4d7dead97dd112b&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=2c5a538d9aa14367a4d7dead97dd112b&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/charts/api.py **Line:** 502:506 **Comment:** *Custom Rule: Add explicit type annotations for the newly introduced version-info local variables to satisfy the type-hint requirement for relevant 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%2F41176&comment_hash=4186fbe10d35d2ca27f0cd64864583cef28796d52a714516baec967d421ebd10&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=4186fbe10d35d2ca27f0cd64864583cef28796d52a714516baec967d421ebd10&reaction=dislike'>π</a> ########## superset/commands/dashboard/update.py: ########## @@ -59,23 +59,31 @@ def __init__(self, model_id: int, data: dict[str, Any]): def run(self) -> Model: self.validate() assert self._model is not None - self.process_tab_diff() - self.process_native_filter_diff() - - # Update tags - if (tags := self._properties.pop("tags", None)) is not None: - update_tags(ObjectType.dashboard, self._model.id, self._model.tags, tags) - - # Re-serialize position_json to escape 4-byte Unicode characters - if position_json := self._properties.get("position_json"): - self._properties["position_json"] = json.dumps(json.loads(position_json)) - - dashboard = DashboardDAO.update(self._model, self._properties) - if self._properties.get("json_metadata"): - DashboardDAO.set_dash_metadata( - dashboard, - data=json.loads(self._properties.get("json_metadata", "{}")), - ) + # Suppress autoflush during the update body so that Continuum's + # before_flush baseline listener does not fire mid-operation while + # the session is only partially populated. + with db.session.no_autoflush: + self.process_tab_diff() + self.process_native_filter_diff() + + # Update tags + if (tags := self._properties.pop("tags", None)) is not None: + update_tags( + ObjectType.dashboard, self._model.id, self._model.tags, tags + ) + + # Re-serialize position_json to escape 4-byte Unicode characters + if position_json := self._properties.get("position_json"): + self._properties["position_json"] = json.dumps( + json.loads(position_json) + ) + + dashboard = DashboardDAO.update(self._model, self._properties) Review Comment: **Suggestion:** Add an explicit type annotation for this newly introduced local variable so the modified code complies with the required type-hinting rule for relevant variables. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The modified code introduces a new local variable without an explicit type annotation, and the stated rule requires type hints for relevant variables that can be annotated. This is a real violation in the shown new hunk. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a45ee18d8a464f3faf1a8c551ad7adb5&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=a45ee18d8a464f3faf1a8c551ad7adb5&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/commands/dashboard/update.py **Line:** 81:81 **Comment:** *Custom Rule: Add an explicit type annotation for this newly introduced local variable so the modified code complies with the required type-hinting rule for relevant variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=09473fc001bf2186b32bde113db79ac7ee9330cedcae956564e7fdf30e3acc58&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=09473fc001bf2186b32bde113db79ac7ee9330cedcae956564e7fdf30e3acc58&reaction=dislike'>π</a> ########## superset/commands/dashboard/importers/v1/__init__.py: ########## @@ -201,7 +205,31 @@ def _import( overwrite or (dashboard.id, chart_id) not in existing_relationships ): - dashboard_chart_ids.append((dashboard.id, chart_id)) + target_chart_ids.append(chart_id) + + if overwrite: + # Replace the dashboard's chart membership entirely. + dashboard.slices = ( + db.session.query(Slice) + .filter(Slice.id.in_(target_chart_ids)) + .all() + if target_chart_ids + else [] + ) + # Flush eagerly so the M2M rows land in + # ``dashboard_slices`` before any subsequent + # autoflush fires an inner-flush event handler + # that would reset the relationship change. + db.session.flush() + elif target_chart_ids: + # Append only the new associations to existing ones. + new_slices = ( + db.session.query(Slice) + .filter(Slice.id.in_(target_chart_ids)) + .all() + ) Review Comment: **Suggestion:** Add an explicit type annotation for the intermediate query result variable so this newly introduced variable complies with the required type-hinting rule. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The rule requires type hints on newly introduced Python variables that can be annotated. `new_slices` is a newly added local variable with an inferable type (`list[Slice]`) but no annotation is present, so this is a real type-hint omission. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57ef2120d19a4039817c8becbd7d4db6&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=57ef2120d19a4039817c8becbd7d4db6&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/commands/dashboard/importers/v1/__init__.py **Line:** 226:230 **Comment:** *Custom Rule: Add an explicit type annotation for the intermediate query result variable so this newly introduced variable complies with the required type-hinting rule. 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=3a9e4985b5c1f9679157300526beac05a35a0ef5e0bf36d3dd4b74d2a91745f6&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=3a9e4985b5c1f9679157300526beac05a35a0ef5e0bf36d3dd4b74d2a91745f6&reaction=dislike'>π</a> ########## scripts/seed_junction_load.py: ########## @@ -0,0 +1,679 @@ +#!/usr/bin/env python3 +# 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. +# +# ---------------------------------------------------------------------- +# Stress-test data generator for the composite-PK migration (sc-105349). +# +# Bulk-inserts synthetic parent rows and many-to-many junction rows for +# the eight association tables that the composite-PK migration touches. +# Useful for measuring migration runtime at varying scales β run this at +# 100K / 1M / 5M / 10M rows and time the migration at each scale to +# verify the O(N log N) extrapolation. +# +# Idempotent: rerunning with the same target is a no-op; rerunning with +# a higher target adds rows up to the new total. Batched bulk INSERTs +# (10K rows per statement) make it fast on Postgres, MySQL, and SQLite. +# +# Usage (inside the Superset container): +# +# docker exec superset-superset-1 \\ +# /app/.venv/bin/python /app/scripts/seed_junction_load.py \\ +# --dashboard-slices 1000000 \\ +# --slice-user 100000 \\ +# --dashboard-user 100000 +# +# Run with no flags for the defaults shown below. Use ``--dry-run`` to +# print the planned inserts without writing anything. +# +# The script connects via Superset's standard ``DATABASE_*`` env vars +# (or ``SUPERSET__SQLALCHEMY_DATABASE_URI`` if set), so it works +# automatically inside the Superset container regardless of which +# metadata DB backend is in use. + +from __future__ import annotations + +import argparse +import logging +import os +import sys +import time +from contextlib import contextmanager +from typing import Iterator +from uuid import uuid4 + +import sqlalchemy as sa +from sqlalchemy.engine import Connection, Engine + +logger = logging.getLogger("seed_junction_load") + +# Bulk INSERT batch size. Larger values = fewer statements but more memory. +BATCH = 10_000 + +# Default per-junction-table target row counts. Tuned to mimic the shape +# of a large multi-team Superset install. Override via CLI flags. +DEFAULTS: dict[str, int] = { + "dashboard_slices": 1_000_000, + "slice_user": 100_000, + "dashboard_user": 100_000, + "dashboard_roles": 10_000, +} + +# (junction_table, fk1_col, fk2_col, parent1_table, parent2_table) +# parents reference id columns; we generate (fk1, fk2) pairs by sampling +# from the parents' existing IDs. +JUNCTIONS: list[tuple[str, str, str, str, str]] = [ + ("dashboard_slices", "dashboard_id", "slice_id", "dashboards", "slices"), + ("slice_user", "user_id", "slice_id", "ab_user", "slices"), + ("dashboard_user", "user_id", "dashboard_id", "ab_user", "dashboards"), + ("dashboard_roles", "dashboard_id", "role_id", "dashboards", "ab_role"), +] + +# Junction tables that originally carried ``UNIQUE(fk1, fk2)`` and therefore +# cannot accept duplicate ``(fk1, fk2)`` pairs even on the pre-migration +# (downgrade) schema. The other JUNCTIONS allow duplicates pre-migration. +# Only ``dashboard_slices`` is listed: the migration's other UNIQUE table +# (``report_schedule_user``) is not in JUNCTIONS β this script doesn't seed +# it β so listing it here would imply coverage that doesn't exist. Add it +# alongside a JUNCTIONS entry if that table ever gets seeded. +JUNCTIONS_WITH_UNIQUE: set[str] = {"dashboard_slices"} + + +# ---------------------------------------------------------------------- +# Connection setup +# ---------------------------------------------------------------------- + + +def build_engine() -> Engine: + """Build a SQLAlchemy engine from Superset env vars.""" + if uri := os.environ.get("SUPERSET__SQLALCHEMY_DATABASE_URI"): + logger.info("Using SUPERSET__SQLALCHEMY_DATABASE_URI from env") + return sa.create_engine(uri) + + try: + dialect = os.environ["DATABASE_DIALECT"] + user = os.environ["DATABASE_USER"] + password = os.environ["DATABASE_PASSWORD"] + host = os.environ["DATABASE_HOST"] + port = os.environ["DATABASE_PORT"] + db = os.environ["DATABASE_DB"] + except KeyError as exc: + sys.exit( + f"Missing env var {exc}; either set DATABASE_DIALECT/USER/PASSWORD/" + f"HOST/PORT/DB or SUPERSET__SQLALCHEMY_DATABASE_URI before running." + ) + + uri = f"{dialect}://{user}:{password}@{host}:{port}/{db}" + logger.info( + "Built URI from DATABASE_* env vars (dialect=%s, host=%s)", dialect, host + ) + return sa.create_engine(uri) + + +# ---------------------------------------------------------------------- +# Helpers +# ---------------------------------------------------------------------- + + +def uuid_value(dialect_name: str) -> bytes | str: + """Return a UUID in the form the active dialect expects. + + MySQL stores UUIDs as ``BINARY(16)`` (16 raw bytes); Postgres has a + native ``UUID`` type that accepts strings; SQLite stores them as + BLOB/TEXT and accepts either. Branching here keeps the seed script + backend-agnostic without depending on Superset's custom column types. + """ + if dialect_name.startswith("mysql"): + return uuid4().bytes + return str(uuid4()) + + +@contextmanager +def time_phase(name: str) -> Iterator[None]: + """Log elapsed wall time for a named phase.""" + start = time.monotonic() + logger.info("[%s] starting", name) + try: + yield + finally: + elapsed = time.monotonic() - start + logger.info("[%s] done in %.2fs", name, elapsed) + + +def count_rows(conn: Connection, table: str) -> int: + return conn.scalar(sa.text(f"SELECT COUNT(*) FROM {table}")) or 0 # noqa: S608 + + +def existing_ids(conn: Connection, table: str, limit: int | None = None) -> list[int]: + sql = f"SELECT id FROM {table} ORDER BY id" # noqa: S608 + if limit is not None: + sql += f" LIMIT {limit}" + return [row[0] for row in conn.execute(sa.text(sql))] + + +# ---------------------------------------------------------------------- +# Parent seeders +# +# Each function ensures the named parent table has at least ``target`` +# rows by inserting synthetic ones with minimal-but-valid columns. +# Returns nothing; subsequent code reads back IDs via ``existing_ids``. +# ---------------------------------------------------------------------- + + +def seed_dashboards(conn: Connection, target: int, dry_run: bool) -> None: + current = count_rows(conn, "dashboards") + if current >= target: + logger.info( + "dashboards: %d rows (target %d) β no insert needed", current, target + ) + return + needed = target - current + logger.info("dashboards: %d β %d (+%d)", current, target, needed) + if dry_run: + return + + dialect = conn.engine.dialect.name + sql = sa.text( + "INSERT INTO dashboards (uuid, dashboard_title, slug, published) " + "VALUES (:uuid, :title, :slug, :published)" + ) + for batch_start in range(0, needed, BATCH): + rows = [ + { + "uuid": uuid_value(dialect), + "title": f"seed_dashboard_{current + i}", + "slug": f"seed-dashboard-{current + i}-{uuid4().hex[:8]}", + "published": False, + } + for i in range(batch_start, min(batch_start + BATCH, needed)) + ] + conn.execute(sql, rows) + logger.info(" dashboards: inserted %d / %d", batch_start + len(rows), needed) + + +def seed_dbs(conn: Connection, dry_run: bool) -> int: + """Ensure at least one row exists in ``dbs`` (parent of ``tables``). + Returns the id to use as ``database_id`` when seeding ``tables``.""" + ids = existing_ids(conn, "dbs", limit=1) + if ids: + return ids[0] + if dry_run: + return -1 # placeholder + dialect = conn.engine.dialect.name + logger.info("dbs: inserting one synthetic database (no rows present)") + conn.execute( + sa.text( + "INSERT INTO dbs (uuid, database_name, sqlalchemy_uri, expose_in_sqllab) " + "VALUES (:uuid, :name, :uri, :expose)" + ), + { + "uuid": uuid_value(dialect), + "name": f"seed_db_{uuid4().hex[:8]}", + "uri": "sqlite:///seed.db", + "expose": False, + }, + ) + return existing_ids(conn, "dbs", limit=1)[0] + + +def seed_tables(conn: Connection, target: int, dry_run: bool) -> None: + current = count_rows(conn, "tables") + if current >= target: + logger.info("tables: %d rows (target %d) β no insert needed", current, target) + return + needed = target - current + logger.info("tables: %d β %d (+%d)", current, target, needed) + if dry_run: + return + + database_id = seed_dbs(conn, dry_run=False) + dialect = conn.engine.dialect.name + sql = sa.text( + "INSERT INTO tables (uuid, table_name, database_id) " + "VALUES (:uuid, :name, :db_id)" + ) + for batch_start in range(0, needed, BATCH): + rows = [ + { + "uuid": uuid_value(dialect), + "name": f"seed_table_{current + i}", + "db_id": database_id, + } + for i in range(batch_start, min(batch_start + BATCH, needed)) + ] + conn.execute(sql, rows) + logger.info(" tables: inserted %d / %d", batch_start + len(rows), needed) + + +def seed_slices(conn: Connection, target: int, dry_run: bool) -> None: + current = count_rows(conn, "slices") + if current >= target: + logger.info("slices: %d rows (target %d) β no insert needed", current, target) + return + needed = target - current + logger.info("slices: %d β %d (+%d)", current, target, needed) + if dry_run: + return + + # Slices reference tables.id; ensure at least one ``tables`` row exists + # so the FK is satisfiable (datasource_id is nullable but we set it for + # realism). The migration test doesn't care, but a real Superset that + # re-renders these slices does. + seed_tables(conn, target=1, dry_run=False) + table_id = existing_ids(conn, "tables", limit=1)[0] + dialect = conn.engine.dialect.name + sql = sa.text( + "INSERT INTO slices " + "(uuid, slice_name, datasource_id, datasource_type, viz_type) " + "VALUES (:uuid, :name, :ds_id, :ds_type, :viz)" + ) + for batch_start in range(0, needed, BATCH): + rows = [ + { + "uuid": uuid_value(dialect), + "name": f"seed_slice_{current + i}", + "ds_id": table_id, + "ds_type": "table", + "viz": "table", + } + for i in range(batch_start, min(batch_start + BATCH, needed)) + ] + conn.execute(sql, rows) + logger.info(" slices: inserted %d / %d", batch_start + len(rows), needed) + + +def seed_users(conn: Connection, target: int, dry_run: bool) -> None: + current = count_rows(conn, "ab_user") + if current >= target: + logger.info("ab_user: %d rows (target %d) β no insert needed", current, target) + return + needed = target - current + logger.info("ab_user: %d β %d (+%d)", current, target, needed) + if dry_run: + return + + sql = sa.text( + "INSERT INTO ab_user (first_name, last_name, username, email, active) " + "VALUES (:first, :last, :username, :email, :active)" + ) + for batch_start in range(0, needed, BATCH): + rows = [ + { + "first": "seed", + "last": f"user_{current + i}", + "username": f"seed_user_{current + i}_{uuid4().hex[:8]}", + "email": f"seed_user_{current + i}_{uuid4().hex[:8]}@example.invalid", + "active": True, + } + for i in range(batch_start, min(batch_start + BATCH, needed)) + ] + conn.execute(sql, rows) + logger.info(" ab_user: inserted %d / %d", batch_start + len(rows), needed) + + +def seed_roles(conn: Connection, target: int, dry_run: bool) -> None: + current = count_rows(conn, "ab_role") + if current >= target: + logger.info("ab_role: %d rows (target %d) β no insert needed", current, target) + return + needed = target - current + logger.info("ab_role: %d β %d (+%d)", current, target, needed) + if dry_run: + return + + sql = sa.text("INSERT INTO ab_role (name) VALUES (:name)") + for batch_start in range(0, needed, BATCH): + rows = [ + {"name": f"seed_role_{current + i}_{uuid4().hex[:8]}"} + for i in range(batch_start, min(batch_start + BATCH, needed)) + ] + conn.execute(sql, rows) + logger.info(" ab_role: inserted %d / %d", batch_start + len(rows), needed) + + +# ---------------------------------------------------------------------- +# Junction seeder +# ---------------------------------------------------------------------- + + +def _load_existing_pairs( + conn: Connection, junction: str, fk1_col: str, fk2_col: str +) -> set[tuple[int, int]]: + """Load existing ``(fk1, fk2)`` pairs from a junction table into a set. + + Used so the seeder can skip them when generating new pairs (junction + tables enforce uniqueness on the FK pair). Memory is ~32 bytes/tuple + on CPython, so 10M existing pairs is ~320MB β acceptable for a dev + machine. The junction / column names come from ``JUNCTIONS``, not + user input, so the f-string interpolation is safe. + """ + sql_text = f"SELECT {fk1_col}, {fk2_col} FROM {junction}" # noqa: S608 + return {(row[0], row[1]) for row in conn.execute(sa.text(sql_text))} + + +def _generate_new_pairs( + p1_ids: list[int], + p2_ids: list[int], + existing_pairs: set[tuple[int, int]], +) -> Iterator[tuple[int, int]]: + """Yield ``(fk1, fk2)`` pairs from the parent1 Γ parent2 cross-product + that are not already in ``existing_pairs``.""" + for fk1 in p1_ids: + for fk2 in p2_ids: + if (fk1, fk2) not in existing_pairs: + yield (fk1, fk2) + + +def seed_junction( + conn: Connection, + junction: str, + fk1_col: str, + fk2_col: str, + parent1: str, + parent2: str, + target: int, + dry_run: bool, +) -> None: + """Bulk-insert junction rows up to ``target`` rows total. + + Generates ``(fk1, fk2)`` pairs by walking the cross-product of + parent1 IDs Γ parent2 IDs in row-major order, skipping pairs that + already exist. Walking the cross-product deterministically keeps + the script replayable: re-running with the same target is a no-op, + and re-running with a higher target appends new pairs in a stable + order regardless of how many runs preceded. + """ + current = count_rows(conn, junction) + if current >= target: + logger.info( + "%s: %d rows (target %d) β no insert needed", junction, current, target + ) + return + needed = target - current + logger.info("%s: %d β %d (+%d)", junction, current, target, needed) + if dry_run: + return + + p1_ids = existing_ids(conn, parent1) + p2_ids = existing_ids(conn, parent2) + max_pairs = len(p1_ids) * len(p2_ids) + if max_pairs < target: + sys.exit( + f"Cannot reach {target} rows in {junction}: " + f"only {max_pairs} unique pairs available " + f"({len(p1_ids)} Γ {len(p2_ids)}). " + f"Increase parent targets and rerun." + ) + + existing_pairs: set[tuple[int, int]] = ( + _load_existing_pairs(conn, junction, fk1_col, fk2_col) if current > 0 else set() + ) + if existing_pairs: + logger.info( + " %s: loaded %d existing pairs into avoidance set", + junction, + len(existing_pairs), + ) + + insert_sql = sa.text( + f"INSERT INTO {junction} ({fk1_col}, {fk2_col}) " # noqa: S608 + f"VALUES (:fk1, :fk2)" + ) + + inserted = 0 + batch: list[dict[str, int]] = [] + for fk1, fk2 in _generate_new_pairs(p1_ids, p2_ids, existing_pairs): + batch.append({"fk1": fk1, "fk2": fk2}) + inserted += 1 + if len(batch) == BATCH or inserted == needed: + conn.execute(insert_sql, batch) + logger.info(" %s: inserted %d / %d", junction, inserted, needed) + batch = [] + if inserted == needed: + return + if inserted < needed: + sys.exit( + f"Ran out of unique pairs at {inserted}/{needed} for {junction} " + f"(parents have {len(p1_ids)} Γ {len(p2_ids)} = {max_pairs} pairs, " + f"{len(existing_pairs)} already present)" + ) + + +# ---------------------------------------------------------------------- +# Orchestration +# ---------------------------------------------------------------------- + + +def _compute_parent_requirements(targets: dict[str, int]) -> dict[str, int]: + """For each parent table, return the minimum row count needed so that + parent1 Γ parent2 β₯ target for every junction it participates in. + + Allocates ceil(sqrt(target)) rows per parent, balanced across the two + parents of each junction. The actual junction seeder will then walk + the cross-product to produce the target number of unique pairs. + """ + parent_req: dict[str, int] = {} + for junction, _, _, p1, p2 in JUNCTIONS: + target = targets.get(junction, 0) + if target == 0: + continue + sqrt_n = int(target**0.5) + 1 + parent_req[p1] = max(parent_req.get(p1, 0), sqrt_n) + parent_req[p2] = max(parent_req.get(p2, 0), sqrt_n) + return parent_req + + +def _seed_parents(conn: Connection, parent_req: dict[str, int], dry_run: bool) -> None: + """Seed parent tables in dependency order: + independent parents (ab_user, ab_role) first, then dashboards / slices / + tables (which transitively depend on dbs, seeded inside seed_tables).""" + if "ab_user" in parent_req: + seed_users(conn, parent_req["ab_user"], dry_run) + if "ab_role" in parent_req: + seed_roles(conn, parent_req["ab_role"], dry_run) + if "dashboards" in parent_req: + seed_dashboards(conn, parent_req["dashboards"], dry_run) + if "slices" in parent_req: + seed_slices(conn, parent_req["slices"], dry_run) + if "tables" in parent_req: + seed_tables(conn, parent_req["tables"], dry_run) + + +def _seed_all_junctions( + conn: Connection, targets: dict[str, int], dry_run: bool +) -> None: + for junction, fk1, fk2, p1, p2 in JUNCTIONS: + target = targets.get(junction, 0) + if target == 0: + continue + with time_phase(f"junction:{junction}"): + seed_junction(conn, junction, fk1, fk2, p1, p2, target, dry_run) + + +def inject_duplicates( + conn: Connection, + junction: str, + fk1_col: str, + fk2_col: str, + pct: float, + dry_run: bool, +) -> None: + """Insert duplicate ``(fk1, fk2)`` rows on a non-UNIQUE junction table. + + Used to stress-test the migration's ``_dedupe_by_min_id`` phase, which + is otherwise a no-op on cleanly-seeded data. Computes ``count = + current_rows * pct / 100`` and inserts that many rows by re-sampling + existing ``(fk1, fk2)`` pairs in row-major order. The synthetic + duplicates land on top of distinct existing pairs (one duplicate per + distinct pair, then wraps), so the migration's dedupe finds and + deletes them. + + **Pre-condition: the table must NOT have UNIQUE on (fk1, fk2)**, i.e., + the schema must be the pre-migration shape (after running + ``superset db downgrade``). On the post-migration schema the composite + PK rejects duplicates and this function will error. + """ + if pct == 0: + return + current = count_rows(conn, junction) + count = int(current * pct / 100) + if count == 0: + logger.info( + "%s: 0 duplicates to inject (current=%d, pct=%g)", + junction, + current, + pct, + ) + return + logger.info( + "%s: injecting %d duplicate rows (%g%% of %d existing)", + junction, + count, + pct, + current, + ) + if dry_run: + return + + select_sql = sa.text( + f"SELECT {fk1_col}, {fk2_col} FROM {junction} ORDER BY id LIMIT :n" # noqa: S608 + ) + sample = conn.execute(select_sql, {"n": count}).fetchall() + if not sample: + logger.warning("%s: no rows to duplicate (table is empty)", junction) + return + + insert_sql = sa.text( + f"INSERT INTO {junction} ({fk1_col}, {fk2_col}) " # noqa: S608 + f"VALUES (:fk1, :fk2)" + ) + inserted = 0 + while inserted < count: + batch: list[dict[str, int]] = [] + while len(batch) < BATCH and inserted < count: + row = sample[inserted % len(sample)] + batch.append({"fk1": row[0], "fk2": row[1]}) + inserted += 1 + conn.execute(insert_sql, batch) + logger.info(" %s: injected %d / %d duplicates", junction, inserted, count) + + +def _inject_dirty_data(conn: Connection, dirty_pct: float, dry_run: bool) -> None: + """Inject duplicate rows on every non-UNIQUE seeded junction. + + The two tables that originally carried ``UNIQUE(fk1, fk2)`` are + skipped because their composite-PK successor (and their pre-migration + UNIQUE constraint) both reject duplicate inserts. + """ + if dirty_pct == 0: + return + for junction, fk1, fk2, _, _ in JUNCTIONS: + if junction in JUNCTIONS_WITH_UNIQUE: + logger.info( + "%s: skipping duplicate injection (table has UNIQUE on FK pair)", + junction, + ) + continue + with time_phase(f"dirty:{junction}"): + inject_duplicates(conn, junction, fk1, fk2, dirty_pct, dry_run) + + +def run(targets: dict[str, int], dry_run: bool, dirty_duplicates_pct: float) -> None: + engine = build_engine() + with engine.begin() as conn: + parent_req = _compute_parent_requirements(targets) + logger.info("Required parent row counts: %s", parent_req) + + with time_phase("parents"): + _seed_parents(conn, parent_req, dry_run) + + with time_phase("junctions"): + _seed_all_junctions(conn, targets, dry_run) + + if dirty_duplicates_pct > 0: + with time_phase("dirty-duplicates"): + _inject_dirty_data(conn, dirty_duplicates_pct, dry_run) + + +# ---------------------------------------------------------------------- +# CLI +# ---------------------------------------------------------------------- + + +def main() -> None: + parser = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) Review Comment: **Suggestion:** Add an explicit type annotation for this parser variable since its type is known. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The local variable `parser` is created without an annotation in new Python code, and its type is clear enough to annotate, so this is a valid type-hint omission under the rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=488286439e0c422194ffb72b512ac03b&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=488286439e0c422194ffb72b512ac03b&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:** scripts/seed_junction_load.py **Line:** 617:621 **Comment:** *Custom Rule: Add an explicit type annotation for this parser variable since its type is known. 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=70ae553edabb9b1543c050a22c9b8d0daf017ac448601349b978d97c50bf752a&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=70ae553edabb9b1543c050a22c9b8d0daf017ac448601349b978d97c50bf752a&reaction=dislike'>π</a> ########## superset/daos/dataset.py: ########## @@ -275,6 +275,103 @@ def update( return super().update(item, attributes) + @classmethod + def _validate_column_date_formats( + cls, property_columns: list[dict[str, Any]] + ) -> None: + for column in property_columns: + if column.get("python_date_format") is None: + continue + if not DatasetDAO.validate_python_date_format(column["python_date_format"]): + raise ValueError( + "python_date_format is an invalid date/timestamp format." + ) + + @classmethod + def _override_columns( + cls, model: SqlaTable, property_columns: list[dict[str, Any]] + ) -> None: + """Replace columns by natural key (``column_name``) β update in place + rather than delete-and-reinsert. + + SPIKE (full-Continuum): the previous + delete-and-reinsert pattern produced overlapping shadow rows in + ``table_columns_version`` (the same ``column_name`` had a DELETE + shadow at tx N alongside an INSERT shadow at tx N for a fresh PK). + Continuum's ``Reverter`` couldn't unwind this on restore: its flush + ordering inserts the historical row before deleting the live one, + hitting the ``UNIQUE (table_id, column_name)`` constraint mid-flush + (ADR-004 Failure 1). + + The natural-key upsert keeps PKs stable across metadata refresh. + Continuum captures only real field changes; new columns get plain + INSERT shadows; removed columns get plain DELETE shadows. No + natural-key collisions, so Reverter can restore cleanly. + + Behaviour change vs. the previous implementation: PKs of unchanged + columns are preserved. Charts that reference columns by their + ``id`` continue to work across a metadata refresh β previously + such references would be invalidated. + """ + existing_by_name = {c.column_name: c for c in model.columns} + incoming_by_name = {p["column_name"]: p for p in property_columns} Review Comment: **Suggestion:** Add explicit type annotations for these newly introduced mapping variables to satisfy the type-hint requirement for relevant local variables. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> These newly added local dictionaries are inferred only from their comprehensions and have no explicit type annotations, so they match the custom ruleβs requirement to flag newly introduced relevant variables that can be annotated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=713c3cd7de794dabaca42793b483f896&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=713c3cd7de794dabaca42793b483f896&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/daos/dataset.py **Line:** 316:317 **Comment:** *Custom Rule: Add explicit type annotations for these newly introduced mapping variables 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%2F41176&comment_hash=4d0db3c63b47284c664a02199e0c4f02ec3938d77283bfaddc82362478c3935a&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=4d0db3c63b47284c664a02199e0c4f02ec3938d77283bfaddc82362478c3935a&reaction=dislike'>π</a> ########## superset/daos/dataset.py: ########## @@ -373,28 +424,22 @@ def update_metrics( if "id" in properties } Review Comment: **Suggestion:** Add explicit type annotations for these newly added local variables in metric upsert logic to comply with mandatory typing conventions. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> These newly added mapping locals are also unannotated despite being obvious candidates for type annotations, so the rule applies here as well. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37db0f5c8d7844da9728b450a2008852&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=37db0f5c8d7844da9728b450a2008852&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/daos/dataset.py **Line:** 419:425 **Comment:** *Custom Rule: Add explicit type annotations for these newly added local variables in metric upsert logic to comply with mandatory typing conventions. 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=aca73c933b382f582bf67ce9c7b8bacd2f3358e435f99ae521c4182303c0daa9&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=aca73c933b382f582bf67ce9c7b8bacd2f3358e435f99ae521c4182303c0daa9&reaction=dislike'>π</a> ########## superset/daos/dataset.py: ########## @@ -275,6 +275,103 @@ def update( return super().update(item, attributes) + @classmethod + def _validate_column_date_formats( + cls, property_columns: list[dict[str, Any]] + ) -> None: + for column in property_columns: + if column.get("python_date_format") is None: + continue + if not DatasetDAO.validate_python_date_format(column["python_date_format"]): + raise ValueError( + "python_date_format is an invalid date/timestamp format." + ) + + @classmethod + def _override_columns( + cls, model: SqlaTable, property_columns: list[dict[str, Any]] + ) -> None: + """Replace columns by natural key (``column_name``) β update in place + rather than delete-and-reinsert. + + SPIKE (full-Continuum): the previous + delete-and-reinsert pattern produced overlapping shadow rows in + ``table_columns_version`` (the same ``column_name`` had a DELETE + shadow at tx N alongside an INSERT shadow at tx N for a fresh PK). + Continuum's ``Reverter`` couldn't unwind this on restore: its flush + ordering inserts the historical row before deleting the live one, + hitting the ``UNIQUE (table_id, column_name)`` constraint mid-flush + (ADR-004 Failure 1). + + The natural-key upsert keeps PKs stable across metadata refresh. + Continuum captures only real field changes; new columns get plain + INSERT shadows; removed columns get plain DELETE shadows. No + natural-key collisions, so Reverter can restore cleanly. + + Behaviour change vs. the previous implementation: PKs of unchanged + columns are preserved. Charts that reference columns by their + ``id`` continue to work across a metadata refresh β previously + such references would be invalidated. + """ + existing_by_name = {c.column_name: c for c in model.columns} + incoming_by_name = {p["column_name"]: p for p in property_columns} + + # Identity is the natural key here, never the payload's ``id``: + # setattr-ing an incoming ``id`` onto a name-matched row would + # rewrite a live primary key, and a renamed column whose payload + # still carries its old ``id`` would INSERT with a live PK while + # the old-named row is deleted in the same flush β INSERTs flush + # before DELETEs, so that collides on the PK / UNIQUE(table_id, + # column_name) constraints. ``table_id`` is pinned to *model*. + protected_keys = ("id", "table_id") + + # Update columns present in both: in-place setattr. + for name, col in existing_by_name.items(): + if name in incoming_by_name: + for key, value in incoming_by_name[name].items(): + if key not in protected_keys: + setattr(col, key, value) + + # Insert columns present only in incoming. + for name, properties in incoming_by_name.items(): + if name not in existing_by_name: + cleaned = { + key: value + for key, value in properties.items() + if key not in protected_keys + } + db.session.add(TableColumn(**{**cleaned, "table_id": model.id})) + + # Delete columns present only in existing. + for name, col in existing_by_name.items(): + if name not in incoming_by_name: + db.session.delete(col) + + @classmethod + def _upsert_columns( + cls, model: SqlaTable, property_columns: list[dict[str, Any]] + ) -> None: + columns_by_id = {column.id: column for column in model.columns} + property_columns_by_id = { + properties["id"]: properties + for properties in property_columns + if "id" in properties + } Review Comment: **Suggestion:** Add explicit type annotations for these new local dictionaries so the modified method fully complies with required typing. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> These are newly introduced local mapping variables without annotations, and they are clearly type-annotatable. That is a direct match for the Python type-hint rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5a6c79d20f2445a1a77019d19cf58f3d&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=5a6c79d20f2445a1a77019d19cf58f3d&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/daos/dataset.py **Line:** 354:359 **Comment:** *Custom Rule: Add explicit type annotations for these new local dictionaries so the modified method fully complies with required typing. 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=32754d2676e26ec11f196406b20936bddf5f25f9f661079b7ae31379e1799061&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=32754d2676e26ec11f196406b20936bddf5f25f9f661079b7ae31379e1799061&reaction=dislike'>π</a> ########## superset/daos/dataset.py: ########## @@ -275,6 +275,103 @@ def update( return super().update(item, attributes) + @classmethod + def _validate_column_date_formats( + cls, property_columns: list[dict[str, Any]] + ) -> None: + for column in property_columns: + if column.get("python_date_format") is None: + continue + if not DatasetDAO.validate_python_date_format(column["python_date_format"]): + raise ValueError( + "python_date_format is an invalid date/timestamp format." + ) + + @classmethod + def _override_columns( + cls, model: SqlaTable, property_columns: list[dict[str, Any]] + ) -> None: + """Replace columns by natural key (``column_name``) β update in place + rather than delete-and-reinsert. + + SPIKE (full-Continuum): the previous + delete-and-reinsert pattern produced overlapping shadow rows in + ``table_columns_version`` (the same ``column_name`` had a DELETE + shadow at tx N alongside an INSERT shadow at tx N for a fresh PK). + Continuum's ``Reverter`` couldn't unwind this on restore: its flush + ordering inserts the historical row before deleting the live one, + hitting the ``UNIQUE (table_id, column_name)`` constraint mid-flush + (ADR-004 Failure 1). + + The natural-key upsert keeps PKs stable across metadata refresh. + Continuum captures only real field changes; new columns get plain + INSERT shadows; removed columns get plain DELETE shadows. No + natural-key collisions, so Reverter can restore cleanly. + + Behaviour change vs. the previous implementation: PKs of unchanged + columns are preserved. Charts that reference columns by their + ``id`` continue to work across a metadata refresh β previously + such references would be invalidated. + """ + existing_by_name = {c.column_name: c for c in model.columns} + incoming_by_name = {p["column_name"]: p for p in property_columns} + + # Identity is the natural key here, never the payload's ``id``: + # setattr-ing an incoming ``id`` onto a name-matched row would + # rewrite a live primary key, and a renamed column whose payload + # still carries its old ``id`` would INSERT with a live PK while + # the old-named row is deleted in the same flush β INSERTs flush + # before DELETEs, so that collides on the PK / UNIQUE(table_id, + # column_name) constraints. ``table_id`` is pinned to *model*. + protected_keys = ("id", "table_id") + + # Update columns present in both: in-place setattr. + for name, col in existing_by_name.items(): + if name in incoming_by_name: + for key, value in incoming_by_name[name].items(): + if key not in protected_keys: + setattr(col, key, value) + + # Insert columns present only in incoming. + for name, properties in incoming_by_name.items(): + if name not in existing_by_name: + cleaned = { + key: value + for key, value in properties.items() + if key not in protected_keys + } + db.session.add(TableColumn(**{**cleaned, "table_id": model.id})) + + # Delete columns present only in existing. + for name, col in existing_by_name.items(): + if name not in incoming_by_name: + db.session.delete(col) + + @classmethod + def _upsert_columns( + cls, model: SqlaTable, property_columns: list[dict[str, Any]] + ) -> None: + columns_by_id = {column.id: column for column in model.columns} + property_columns_by_id = { + properties["id"]: properties + for properties in property_columns + if "id" in properties + } + + for properties in property_columns: + if "id" not in properties: + db.session.add(TableColumn(**{**properties, "table_id": model.id})) + + for properties in property_columns_by_id.values(): + col = columns_by_id[properties["id"]] + for key, value in properties.items(): + setattr(col, key, value) + + ids_to_keep = property_columns_by_id.keys() Review Comment: **Suggestion:** Annotate this new local variable with its expected key-type view (or concrete collection type) to meet the type-hint rule. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> This new local variable is unannotated and its type is not explicit even though it can be annotated, so it falls under the custom type-hint requirement. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b99a3c97c08c4e73a0bf01cc4deea792&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=b99a3c97c08c4e73a0bf01cc4deea792&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/daos/dataset.py **Line:** 370:370 **Comment:** *Custom Rule: Annotate this new local variable with its expected key-type view (or concrete collection type) to meet the type-hint rule. 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=f1c20ea4d454d4fabeaeeec738845f987fabc4d2643ff295d10edb6eb3035723&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=f1c20ea4d454d4fabeaeeec738845f987fabc4d2643ff295d10edb6eb3035723&reaction=dislike'>π</a> ########## superset/daos/version.py: ########## @@ -0,0 +1,69 @@ +# 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. +"""Backward-compat faΓ§ade for the entity-versioning DAO surface. + +The actual implementation lives in :mod:`superset.versioning.queries` +(read side: list/get/resolve/find/UUID derivation). This module +re-exports it under a single ``VersionDAO`` class plus the module-level +UUID helpers so existing callers keep working without changes. (The +write side β restore + audit stamping β ships in a later PR; only the +read surface is wired here.) + +New code should import from the versioning sub-modules directly. +""" + +from __future__ import annotations + +from superset.versioning.queries import ( + current_live_transaction_id, + current_live_version_uuid, + current_version_number, + derive_version_uuid, + derive_version_uuid as _derive_version_uuid, # noqa: F401 + find_active_by_uuid, + get_version, + list_change_records_batch, + list_versions, + resolve_version_uuid, + VERSION_UUID_NAMESPACE, +) + +# Re-exports for ``from superset.daos.version import β¦`` consumers. +__all__ = [ + "VERSION_UUID_NAMESPACE", + "VersionDAO", + "derive_version_uuid", +] Review Comment: **Suggestion:** Add a type annotation to the module-level export list so this new variable is explicitly typed. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The custom rule requires type hints on relevant variables that can be annotated. `__all__` is a new module-level variable and is left untyped here, so the suggestion correctly identifies a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1a62397e53b40a18a6a6de500b999b3&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=b1a62397e53b40a18a6a6de500b999b3&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/daos/version.py **Line:** 46:50 **Comment:** *Custom Rule: Add a type annotation to the module-level export list so this new variable is explicitly typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=274e2d8f94f2b010a9c62f2c81969a3dc85fde8a8c5f731d0aaf83dcf64a8a26&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=274e2d8f94f2b010a9c62f2c81969a3dc85fde8a8c5f731d0aaf83dcf64a8a26&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]
