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: [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]
