You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/18 01:25:38 UTC
[PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
rusackas opened a new pull request, #41179:
URL: https://github.com/apache/superset/pull/41179
### SUMMARY
Forward-compatible groundwork for the SQLAlchemy 1.4 → 2.0 migration (companion to #40830 / #39735, and a sibling of the already-merged #40276 and the in-flight #40275). **Every change here is backward-compatible with the current `sqlalchemy>=1.4,<2` / `flask-sqlalchemy 2.x` baseline**, so it can land on `master` today without bumping the version, while shrinking the eventual bump and de-risking it.
Two categories:
**1. Removed-in-2.0 API fixes** — these break on 2.0 but still work on 1.4:
- `sqlalchemy.orm.eagerload` (a removed alias of `joinedload`) → `joinedload`, in the security manager.
- `sqlalchemy.sql.visitors.VisitableType` (removed) → `sqlalchemy.types.TypeEngine`, in dataset-importer and `mock_data` type hints.
- `flask_sqlalchemy.BaseQuery` (removed in Flask-SQLAlchemy 3.x, which SA 2.0 requires) → imported from `flask_sqlalchemy.query.Query` with a fallback to the FSA 2.x path, in the query / saved-query filters and the report command tests.
- `Engine.execute` / raw-string execution → startup health check wrapped in a `Connection` + `text()`; secrets re-encryptor uses `text()` with a single bind dict and `row._mapping[...]` access.
**2. Prospective Annotated Declarative prep** — no-ops on 1.4, but they ease the eventual 2.0 flip:
- `__allow_unmapped__` on the FAB declarative base so legacy 1.x annotations are tolerated during incremental migration.
- `Mapped[...]` return annotations on `created_by_fk` / `changed_by_fk` and `BaseDatasource.slices`.
The `select()` and `case()` syntax changes from the same effort are intentionally **excluded** here — they already landed via #40276 and are covered by #40275 respectively.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no user-facing changes.
### TESTING INSTRUCTIONS
CI. Locally, all of these were verified against the current `SQLAlchemy 1.4.54` / `Flask-SQLAlchemy 2.5.1` baseline (imports resolve, `BaseQuery` shim falls back correctly, `pre-commit run` is green including mypy/pylint/ruff). They should be a no-op behaviorally on 1.4 and unblock the 2.0 import/boot path.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4774063551
Curious your take on this one @hy144328 - wasn't sure if this is already on your roadmap.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41179:
URL: https://github.com/apache/superset/pull/41179#discussion_r3432627108
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -37,6 +36,13 @@
)
from sqlalchemy.sql import func
+try:
+ # Flask-SQLAlchemy 3.x (required by SQLAlchemy 2.0)
+ from flask_sqlalchemy.query import Query as BaseQuery
+except ImportError: # pragma: no cover
+ # Flask-SQLAlchemy 2.x
+ from flask_sqlalchemy import BaseQuery
Review Comment:
<!-- Bito Reply -->
The decision to avoid a shared shim module for a temporary six-line import fallback is reasonable given the plan to remove it once the dependency is updated. Maintaining the current approach avoids unnecessary architectural complexity for code that is intended to be short-lived.
**tests/integration_tests/reports/commands_tests.py**
```
try:
# Flask-SQLAlchemy 3.x (required by SQLAlchemy 2.0)
from flask_sqlalchemy.query import Query as BaseQuery
except ImportError: # pragma: no cover
# Flask-SQLAlchemy 2.x
from flask_sqlalchemy import BaseQuery
```
##########
superset/connectors/sqla/models.py:
##########
@@ -335,7 +335,7 @@ def is_virtual(self) -> bool:
return self.kind == DatasourceKind.VIRTUAL
@declared_attr
- def slices(self) -> RelationshipProperty:
+ def slices(self) -> Mapped[list["Slice"]]:
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is technically correct regarding the current SQLAlchemy 1.4 behavior, where `@declared_attr` methods returning ORM constructs should be annotated as `RelationshipProperty`. While your intention to prepare for future declarative patterns is noted, using `RelationshipProperty` ensures compatibility with current type checking and avoids potential mypy failures in the existing codebase.
**superset/connectors/sqla/models.py**
```
@declared_attr
def slices(self) -> RelationshipProperty:
return relationship(
"Slice",
overlaps="table",
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4737000151
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4738373401
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4784918612
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "hy144328 (via GitHub)" <gi...@apache.org>.
hy144328 commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4783465278
> Curious your take on this one @hy144328 - wasn't sure if this is already on your roadmap.
@rusackas Here are my two cents:
> * sqlalchemy.orm.eagerload (a removed alias of joinedload) → joinedload, in the security manager.
> * sqlalchemy.sql.visitors.VisitableType (removed) → sqlalchemy.types.TypeEngine, in dataset-importer and mock_data type hints.
These two were not on my roadmap because the unit tests did not flag them. #40273
So it is good to have them covered in this PR. :ok_hand:
> * flask_sqlalchemy.BaseQuery (removed in Flask-SQLAlchemy 3.x, which SA 2.0 requires) → imported from flask_sqlalchemy.query.Query with a fallback to the FSA 2.x path, in the query / saved-query filters and the report command tests.
Flask SQLAlchemy is also not on my radar yet but it looks good to me.
Perhaps, @dpgaspar has a more educated opinion here since he has experience with Flask and #35117 .
> * Engine.execute / raw-string execution → startup health check wrapped in a Connection + text(); secrets re-encryptor uses text() with a single bind dict and row._mapping[...] access.
This one is actually on the roadmap. However, when I grep it, there are many more occurrences that are not covered by unit tests. This is not a problem. I have it on my roadmap, and I will probably crush this warning next.
> * __allow_unmapped__ on the FAB declarative base so legacy 1.x annotations are tolerated during incremental migration.
> * Mapped[...] return annotations on created_by_fk / changed_by_fk and BaseDatasource.slices.
This is actually on my radar since this is also part of official migration guide. https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#migration-to-2-0-step-six-add-allow-unmapped-to-explicitly-typed-orm-models
I suspect that there are even more ORMs to patch than currently flagged by the unit tests.
---
Long story short, I think this PR complements my roadmap well, and crushes multiple warnings that I have not documented yet.
If you do not mind, I will continue with my work, too, because it will take some more manual labor for code not covered by unit tests.
I have to look more carefully at the migration of the SQLAlchemy ORM in particular because they propose both a [quick and dirty way](https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#migration-to-2-0-step-six-add-allow-unmapped-to-explicitly-typed-orm-models) based on `__allow_unmapped__` and a [slow and type-safe way](https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#migrating-an-existing-mapping) based on `Mapped` and `mapped_column()`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4737025399
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41179?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.00000%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.59%. Comparing base ([`883b7a2`](https://app.codecov.io/gh/apache/superset/commit/883b7a286db420d64eb721b53ab02fbb98c2b85a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`18a2a45`](https://app.codecov.io/gh/apache/superset/commit/18a2a4509ea9212c1b56d1c21ed4f877ea319ef0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41179?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/encrypt.py](https://app.codecov.io/gh/apache/superset/pull/41179?src=pr&el=tree&filepath=superset%2Futils%2Fencrypt.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvZW5jcnlwdC5weQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/mock\_data.py](https://app.codecov.io/gh/apache/superset/pull/41179?src=pr&el=tree&filepath=superset%2Futils%2Fmock_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (883b7a2) and HEAD (18a2a45). Click for more details.
>
> <details><summary>HEAD has 5 uploads less than BASE</summary>
>
>| Flag | BASE (883b7a2) | HEAD (18a2a45) |
>|------|------|------|
>|python|7|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41179 +/- ##
==========================================
- Coverage 63.73% 55.59% -8.14%
==========================================
Files 2651 2651
Lines 144737 144743 +6
Branches 33402 33402
==========================================
- Hits 92242 80476 -11766
- Misses 50827 63534 +12707
+ Partials 1668 733 -935
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.32% <80.00%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.91% <80.00%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.97% <80.00%> (-17.37%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41179?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4783822441
Thanks @hy144328, that all tracks. These were just the warnings our unit tests happened to surface, so glad it slots into your roadmap rather than stepping on it. Go for the broader ORM sweep, and ping me if you want a second set of eyes on the `Mapped`/`mapped_column` direction.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41179:
URL: https://github.com/apache/superset/pull/41179
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41179:
URL: https://github.com/apache/superset/pull/41179#discussion_r3432481490
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -37,6 +36,13 @@
)
from sqlalchemy.sql import func
+try:
+ # Flask-SQLAlchemy 3.x (required by SQLAlchemy 2.0)
+ from flask_sqlalchemy.query import Query as BaseQuery
+except ImportError: # pragma: no cover
+ # Flask-SQLAlchemy 2.x
+ from flask_sqlalchemy import BaseQuery
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Import pattern duplicated</b></div>
<div id="fix">
The try/except import pattern (lines 39-44) is duplicated verbatim from `superset/queries/filters.py` (lines 19-24) and `superset/queries/saved_queries/filters.py` (lines 26-29). If the import strategy needs updating for Flask-SQLAlchemy version changes, all three locations must be modified, creating maintenance risk.
</div>
</div>
<small><i>Code Review Run #368617</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/connectors/sqla/models.py:
##########
@@ -335,7 +335,7 @@ def is_virtual(self) -> bool:
return self.kind == DatasourceKind.VIRTUAL
@declared_attr
- def slices(self) -> RelationshipProperty:
+ def slices(self) -> Mapped[list["Slice"]]:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect type annotation for declared_attr</b></div>
<div id="fix">
The return type `Mapped[list["Slice"]]` is incorrect for a `@declared_attr` method in SQLAlchemy 1.4. The `Mapped[...]` annotation style applies to direct class attribute assignments (e.g., `columns: Mapped[list[TableColumn]] = relationship(...)` at line 1304), not to `@declared_attr` methods. For `@declared_attr` methods that return ORM constructs, the return type should be `RelationshipProperty`, as confirmed by the existing pattern at line 1117 (`def datasource(self) -> RelationshipProperty:`). This mismatch can cause mypy failures and incorrect type inference.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -335,7 +335,7 @@ class BaseDatasource(Model, AuditMixinNullable, CertificationMixin,
return self.kind == DatasourceKind.VIRTUAL
@declared_attr
- def slices(self) -> Mapped[list["Slice"]]:
+ def slices(self) -> RelationshipProperty:
return relationship(
"Slice",
overlaps="table",
```
</div>
</details>
</div>
<small><i>Code Review Run #368617</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41179:
URL: https://github.com/apache/superset/pull/41179#discussion_r3432626625
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -37,6 +36,13 @@
)
from sqlalchemy.sql import func
+try:
+ # Flask-SQLAlchemy 3.x (required by SQLAlchemy 2.0)
+ from flask_sqlalchemy.query import Query as BaseQuery
+except ImportError: # pragma: no cover
+ # Flask-SQLAlchemy 2.x
+ from flask_sqlalchemy import BaseQuery
Review Comment:
It is just a six-line import fallback that goes away entirely once we are on FSA 3.x, so I would rather not stand up a shared shim module for it. Happy to dedupe if it outlives the bump.
##########
superset/connectors/sqla/models.py:
##########
@@ -335,7 +335,7 @@ def is_virtual(self) -> bool:
return self.kind == DatasourceKind.VIRTUAL
@declared_attr
- def slices(self) -> RelationshipProperty:
+ def slices(self) -> Mapped[list["Slice"]]:
Review Comment:
This one is intentional 2.0 prep, the whole point is moving toward `Mapped[...]` annotations ahead of the declarative flip. mypy is happy with it on 1.4, so I am inclined to leave it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4775072703
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: make import/expression layer SQLAlchemy 2.0-compatible [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41179:
URL: https://github.com/apache/superset/pull/41179#issuecomment-4776797109
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org