Coerce aliased Select and CompoundSelect to subquery #12433
Conversation
| @@ -1015,6 +1016,19 @@ def _alias_factory( | |||
| flat: bool = False, | |||
| adapt_on_names: bool = False, | |||
| ) -> Union[AliasedClass[_O], FromClause]: | |||
| if isinstance(element, GenerativeSelect): | |||
| util.warn( | |||
There was a problem hiding this comment.
good start. We already have this warning in coercions itself, and adding "subquery()" is in fact a coercion, so coercions can handle this right now like this:
diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index 48282b2d5..c7bcb35c7 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -41,6 +41,7 @@ from ._typing import insp_is_mapper
from ._typing import prop_is_relationship
from .base import _class_to_mapper as _class_to_mapper
from .base import _MappedAnnotationBase
+from ..sql.selectable import GenerativeSelect
from .base import _never_set as _never_set # noqa: F401
from .base import _none_only_set as _none_only_set # noqa: F401
from .base import _none_set as _none_set # noqa: F401
@@ -1015,7 +1016,11 @@ class AliasedInsp(
flat: bool = False,
adapt_on_names: bool = False,
) -> Union[AliasedClass[_O], FromClause]:
- if isinstance(element, FromClause):
+ if isinstance(element, GenerativeSelect):
+ return coercions.expect(
+ roles.FromClauseRole, element, flat=flat, allow_select=True
+ )
+ elif isinstance(element, FromClause):
if adapt_on_names:
raise sa_exc.ArgumentError(
"adapt_on_names only applies to ORM elements"
I dont see any need to distinguish between "named" and "not named", if the user wants a specific subquery name they should call .subquery(name) as is the correct API
There was a problem hiding this comment.
I couldn't get the exact code above to produce the expected result. It appears a GenerativeSelect can't be automatically coerced to FromClause without first turning it into a subquery.
There was a problem hiding this comment.
the coercion itself does the subquery() call. I tested the above patch with the original case at #6274, and if it is not working for an additional case here then I'd want to add that support to coercions, I can do that if you show me the failure
There was a problem hiding this comment.
somethings weird here because this worked in my main branch and isnt working in this PR, checking
| element=select(Point.x).filter(Point.id == 1), | ||
| name="point_alias", | ||
| ) | ||
| eq_(q1.name, "point_alias") |
There was a problem hiding this comment.
I think each of these tests should add a simple self.assert_compile(select(q1), " <expected SQL>") statement just as a sanity check
| f"{type(element).__name__} can't be aliased, " | ||
| f"coercing to subquery instead." | ||
| ) | ||
| return coercions.expect( |
There was a problem hiding this comment.
how about we instead just raise an informative unsupported error here? This is what other cases like this behave in 2.0 so I don't think it makes much sense to add support for a previously unsupported feature (from by understanding this raised an exception in all cases) and also deprecate it right away
There was a problem hiding this comment.
OK we changed from coercion + warning to raise in #10236. So this PR needs to have a separate version for main vs. 2.0. in main, these test should expect a raise. in 2.0, we need to use expect_deprecated. the tests themselves should be in test/orm/test_deprecated.py.
@CaselIT this is getting complicated do we want to just move to gerrit and straighten it out
it's likely the best option |
Fixes issue #6274
Description
When
aliased()is used on a select statement or compound select statement, it currently throws an error. The proposed solution in #6274 was to instead return a subquery and emit a warning to the user that aliases are not supported for these types of statements.Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!