Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ dependencies = [
"sshtunnel>=0.4.0, <0.5",
"simplejson>=3.15.0",
"slack_sdk>=3.19.0, <4",
"sqlalchemy>=1.4, <2",
"sqlalchemy>=2, <3",
"sqlalchemy-utils>=0.38.0, <0.43", # expanding lowerbound to work with pydoris
Comment on lines +102 to 103

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Architect Review — CRITICAL

Bumping SQLAlchemy to >=2 while the codebase still calls Engine.execute (for example db.engine.execute("SELECT 1") in superset/initialization/init.py:40 and engine.execute(...) in multiple db_engine_specs) will cause runtime AttributeError under SQLAlchemy 2, breaking normal app startup and common database operations.

Suggestion: Revert the SQLAlchemy requirement to <2 until all Engine.execute usages are refactored to use Connection.execute/db.session.execute, or include those refactors in this PR and verify core startup and DB engine spec paths under SQLAlchemy 2.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.

**Path:** pyproject.toml
**Line:** 102:103
**Comment:**
	*CRITICAL: Bumping SQLAlchemy to >=2 while the codebase still calls Engine.execute (for example db.engine.execute("SELECT 1") in superset/initialization/__init__.py:40 and engine.execute(...) in multiple db_engine_specs) will cause runtime AttributeError under SQLAlchemy 2, breaking normal app startup and common database operations.

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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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

Comment on lines +102 to 103

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Architect Review — HIGH

Legacy Alembic migration scripts still use SQLAlchemy 1.x-only constructs like MetaData(bind=bind), autoload=True, and select([...]) (for example in superset/migrations/versions/2020-04-24_10-46_e557699a813e_add_tables_relation_to_row_level_.py and 2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py), which are incompatible with SQLAlchemy 2 and will cause db upgrade to fail for fresh installs or long-jump upgrades.

Suggestion: Before enforcing SQLAlchemy>=2, update or patch the affected Alembic revisions (or add compatibility shims in the migration env) to remove MetaData(bind=...), autoload=True, and select([...]) usage, and run a full migration chain test from an empty database under SQLAlchemy 2.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.

**Path:** pyproject.toml
**Line:** 102:103
**Comment:**
	*HIGH: Legacy Alembic migration scripts still use SQLAlchemy 1.x-only constructs like MetaData(bind=bind), autoload=True, and select([...]) (for example in superset/migrations/versions/2020-04-24_10-46_e557699a813e_add_tables_relation_to_row_level_.py and 2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py), which are incompatible with SQLAlchemy 2 and will cause db upgrade to fail for fresh installs or long-jump upgrades.

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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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

"sqlglot>=28.10.0, <29",
# newer pandas needs 0.9+
Expand Down
5 changes: 3 additions & 2 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ flask-migrate==3.1.0
# via apache-superset (pyproject.toml)
flask-session==0.8.0
# via apache-superset (pyproject.toml)
flask-sqlalchemy==2.5.1
flask-sqlalchemy==3.1.1
# via
# flask-appbuilder
# flask-migrate
Expand Down Expand Up @@ -397,7 +397,7 @@ sniffio==1.3.1
# via trio
sortedcontainers==2.4.0
# via trio
sqlalchemy==1.4.54
sqlalchemy==2.0.49
# via
# apache-superset (pyproject.toml)
# alembic
Expand Down Expand Up @@ -439,6 +439,7 @@ typing-extensions==4.15.0
# referencing
# selenium
# shillelagh
# sqlalchemy
# typing-inspection
typing-inspection==0.4.1
# via pydantic
Expand Down
5 changes: 3 additions & 2 deletions requirements/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ flask-session==0.8.0
# via
# -c requirements/base-constraint.txt
# apache-superset
flask-sqlalchemy==2.5.1
flask-sqlalchemy==3.1.1
# via
# -c requirements/base-constraint.txt
# flask-appbuilder
Expand Down Expand Up @@ -957,7 +957,7 @@ sortedcontainers==2.4.0
# via
# -c requirements/base-constraint.txt
# trio
sqlalchemy==1.4.54
sqlalchemy==2.0.49
# via
# -c requirements/base-constraint.txt
# alembic
Expand Down Expand Up @@ -1039,6 +1039,7 @@ typing-extensions==4.15.0
# referencing
# selenium
# shillelagh
# sqlalchemy
# starlette
# typing-inspection
typing-inspection==0.4.1
Expand Down
2 changes: 1 addition & 1 deletion superset-core/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ classifiers = [
dependencies = [
"flask-appbuilder>=5.0.2,<6",
"pydantic>=2.8.0",
"sqlalchemy>=1.4.0,<2.0",
"sqlalchemy>=2,<3.0",
"sqlalchemy-utils>=0.38.0, <0.43", # expanding lowerbound to work with pydoris
"sqlglot>=28.10.0, <29",
"typing-extensions>=4.0.0",
Expand Down
2 changes: 1 addition & 1 deletion superset/commands/importers/v1/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def safe_insert_dashboard_chart_relationships(
# Get existing relationships only for dashboards being updated
dashboard_ids = {dashboard_id for dashboard_id, _ in dashboard_chart_ids}
existing_relationships = db.session.execute(
select([dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id]).where(
select(dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id).where(
dashboard_slices.c.dashboard_id.in_(dashboard_ids)
)
).fetchall()
Expand Down
76 changes: 29 additions & 47 deletions superset/common/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ def add_types_to_charts(

charts = (
select(
[
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
]
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -64,11 +62,9 @@ def add_types_to_dashboards(

dashboards = (
select(
[
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
]
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -96,11 +92,9 @@ def add_types_to_saved_queries(

saved_queries = (
select(
[
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
]
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -128,11 +122,9 @@ def add_types_to_datasets(

datasets = (
select(
[
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
]
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -238,11 +230,9 @@ def add_owners_to_charts(

charts = (
select(
[
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
]
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -274,11 +264,9 @@ def add_owners_to_dashboards(

dashboards = (
select(
[
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
]
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -310,11 +298,9 @@ def add_owners_to_saved_queries(

saved_queries = (
select(
[
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
]
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -346,11 +332,9 @@ def add_owners_to_datasets(

datasets = (
select(
[
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
]
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -440,7 +424,7 @@ def add_owners(metadata: MetaData) -> None:
columns = ["tag_id", "object_id", "object_type"]

# create a custom tag for each user
ids = select([users.c.id])
ids = select(users.c.id)
insert = tag.insert()
for (id_,) in db.session.execute(ids):
with contextlib.suppress(IntegrityError): # already exists
Expand Down Expand Up @@ -478,18 +462,16 @@ def add_favorites(metadata: MetaData) -> None:
columns = ["tag_id", "object_id", "object_type"]

# create a custom tag for each user
ids = select([users.c.id])
ids = select(users.c.id)
insert = tag.insert()
for (id_,) in db.session.execute(ids):
with contextlib.suppress(IntegrityError): # already exists
db.session.execute(insert, name=f"favorited_by:{id_}", type=TagType.type)
favstars = (
select(
[
tag.c.id.label("tag_id"),
favstar.c.obj_id.label("object_id"),
func.lower(favstar.c.class_name).label("object_type"),
]
tag.c.id.label("tag_id"),
favstar.c.obj_id.label("object_id"),
func.lower(favstar.c.class_name).label("object_type"),
)
.select_from(
join(
Expand Down
6 changes: 2 additions & 4 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1695,11 +1695,9 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
# for those we fall back to LIMIT 1.
tbl, _unused_cte = self.get_from_clause(template_processor)
if self.db_engine_spec.type_probe_needs_row:
qry = sa.select([sqla_column]).limit(1).select_from(tbl)
qry = sa.select(sqla_column).limit(1).select_from(tbl)
else:
qry = (
sa.select([sqla_column]).where(sa.false()).select_from(tbl)
)
qry = sa.select(sqla_column).where(sa.false()).select_from(tbl)
sql = self.database.compile_sqla_query(
qry,
catalog=self.catalog,
Expand Down
4 changes: 2 additions & 2 deletions superset/extensions/metadb.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def _build_sql(
"""
Build SQLAlchemy query object.
"""
query = select([self._table])
query = select(self._table)

for column_name, filter_ in bounds.items():
column = self._table.c[column_name]
Expand Down Expand Up @@ -445,7 +445,7 @@ def insert_row(self, row: Row) -> int:
if self._rowid:
return result.inserted_primary_key[0]

query = select([func.count()]).select_from(self._table)
query = select(func.count()).select_from(self._table)
return connection.execute(query).scalar()

@check_dml
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def upgrade():
clusters = sa.Table("clusters", metadata, autoload=True)

statement = datasources.update().values(
cluster_id=sa.select([clusters.c.id])
cluster_id=sa.select(clusters.c.id)
.where(datasources.c.cluster_name == clusters.c.cluster_name)
.as_scalar()
)
Expand Down Expand Up @@ -91,7 +91,7 @@ def downgrade():
clusters = sa.Table("clusters", metadata, autoload=True)

statement = datasources.update().values(
cluster_name=sa.select([clusters.c.cluster_name])
cluster_name=sa.select(clusters.c.cluster_name)
.where(datasources.c.cluster_id == clusters.c.id)
.as_scalar()
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def upgrade():
)

rlsf = sa.Table("row_level_security_filters", metadata, autoload=True)
filter_ids = sa.select([rlsf.c.id, rlsf.c.table_id])
filter_ids = sa.select(rlsf.c.id, rlsf.c.table_id)

for row in bind.execute(filter_ids):
move_table_id = rls_filter_tables.insert().values(
Expand Down Expand Up @@ -85,7 +85,7 @@ def downgrade():
rls_filter_tables = sa.Table("rls_filter_tables", metadata, autoload=True)
rls_filter_roles = sa.Table("rls_filter_roles", metadata, autoload=True)

filter_tables = sa.select([rls_filter_tables.c.rls_filter_id]).group_by(
filter_tables = sa.select(rls_filter_tables.c.rls_filter_id).group_by(
rls_filter_tables.c.rls_filter_id
)

Expand All @@ -95,7 +95,7 @@ def downgrade():
filter_params = dict(bind.execute(filter_query).fetchone())
origin_id = filter_params.pop("id", None)
table_ids = bind.execute(
sa.select([rls_filter_tables.c.table_id]).where(
sa.select(rls_filter_tables.c.table_id).where(
rls_filter_tables.c.rls_filter_id == row["rls_filter_id"]
)
).fetchall()
Expand Down
Loading
Loading