codeant-ai-for-open-source[bot] commented on code in PR #41075: URL: https://github.com/apache/superset/pull/41075#discussion_r3500168839
########## 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. Review Comment: **Suggestion:** The top-level description says the script seeds eight association tables, but `JUNCTIONS` only defines four, which is a direct contradiction and will mislead operators about test coverage and scale assumptions. Update the comment to match the actual seeded table set. [comment mismatch] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx ⚠️ Operators may overestimate migration test table coverage. ⚠️ Script documentation misleading about seeded association tables. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `scripts/seed_junction_load.py` and read the top-level description at lines 20–26; lines 22–23 state that the script “Bulk-inserts … junction rows for the eight association tables that the composite-PK migration touches.” 2. Scroll to the `JUNCTIONS` constant at lines 79–84, which defines the junctions actually seeded: `dashboard_slices`, `slice_user`, `dashboard_user`, and `dashboard_roles` — only four association tables. 3. Inspect `_seed_all_junctions()` at lines 496–505, which loops over `JUNCTIONS` and calls `seed_junction()` for each entry, confirming that only the four tables in `JUNCTIONS` are seeded by this script. 4. Note the comment about other association tables (e.g. `report_schedule_user`) at lines 86–93 and the absence of those tables from `JUNCTIONS`, demonstrating that the “eight association tables” claim in lines 22–23 overstates actual coverage and can mislead operators about which tables are exercised when using this script. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ab549cb09974cbca139a952cc330ded&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=9ab549cb09974cbca139a952cc330ded&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:** 22:23 **Comment:** *Comment Mismatch: The top-level description says the script seeds eight association tables, but `JUNCTIONS` only defines four, which is a direct contradiction and will mislead operators about test coverage and scale assumptions. Update the comment to match the actual seeded table set. 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=c9fe380b773292d5250b6b6523586dfb6d0dc0270e2c4d1418d9b077f35600e7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=c9fe380b773292d5250b6b6523586dfb6d0dc0270e2c4d1418d9b077f35600e7&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) Review Comment: **Suggestion:** Building the SQLAlchemy URL by string interpolation will break when `DATABASE_PASSWORD` (or username/database name) contains reserved URL characters like `@`, `:`, `/`, or `#`, causing connection parsing failures at runtime. Build the URL with SQLAlchemy’s URL helpers (or percent-encode credentials) instead of concatenating raw values. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Stress-test seeding script fails with complex DB passwords. ⚠️ Migration runtime benchmarking blocked in affected environments. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `scripts/seed_junction_load.py` and note `build_engine()` at lines 101–124, which reads `DATABASE_*` env vars (lines 107–113) and interpolates them into `uri` at line 120 before calling `sa.create_engine(uri)` at line 124. 2. In a Superset container or local environment, unset `SUPERSET__SQLALCHEMY_DATABASE_URI` so `build_engine()` uses the `DATABASE_*` variables, and set `DATABASE_DIALECT`, `DATABASE_USER`, `DATABASE_PASSWORD`, `DATABASE_HOST`, `DATABASE_PORT`, and `DATABASE_DB` with a password containing reserved URL characters, e.g. `DATABASE_PASSWORD='p@ss:word'`. 3. Run the script as documented in the header comment at lines 32–38, for example: `python scripts/seed_junction_load.py --dashboard-slices 10 --slice-user 10 --dashboard-user 10`, which invokes `main()` at lines 618–679; `main()` calls `run()` at lines 595–675, and `run()` calls `build_engine()` at line 596. 4. Observe that `build_engine()` constructs a URL like `postgresql://user:p@ss:word@host:5432/db` at line 120, and when `sa.create_engine(uri)` at line 124 parses this string, the unescaped `@`/`:` characters in the password cause the connection URL to be misparsed or rejected, leading to engine creation failure and preventing the seeding run. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=59356e33813049939cb90480755ef0a7&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=59356e33813049939cb90480755ef0a7&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:** 120:124 **Comment:** *Logic Error: Building the SQLAlchemy URL by string interpolation will break when `DATABASE_PASSWORD` (or username/database name) contains reserved URL characters like `@`, `:`, `/`, or `#`, causing connection parsing failures at runtime. Build the URL with SQLAlchemy’s URL helpers (or percent-encode credentials) instead of concatenating raw values. 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=4b56ff1933175f7dd985842c835819150ec655bd748b5c0820fd39bf225a03c0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=4b56ff1933175f7dd985842c835819150ec655bd748b5c0820fd39bf225a03c0&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]
