From 66c6b8558a6b64820b790199816acc66deffdacc Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 5 Dec 2022 16:11:59 -0500 Subject: [PATCH] disable polymorphic adaption in most cases Improved a fix first made in version 1.4 for :ticket:`8456` which scaled back the usage of internal "polymorphic adapters", that are used to render ORM queries when the :paramref:`_orm.Mapper.with_polymorphic` parameter is used. These adapters, which are very complex and error prone, are now used only in those cases where an explicit user-supplied subquery is used for :paramref:`_orm.Mapper.with_polymorphic`, which includes only the use case of concrete inheritance mappings that use the :func:`_orm.polymorphic_union` helper, as well as the legacy use case of using an aliased subquery for joined inheritance mappings, which is not needed in modern use. For the most common case of joined inheritance mappings that use the built-in polymorphic loading scheme, which includes those which make use of the :paramref:`_orm.Mapper.polymorphic_load` parameter set to ``inline``, polymorphic adapters are now no longer used. This has both a positive performance impact on the construction of queries as well as a substantial simplification of the internal query rendering process. The specific issue targeted was to allow a :func:`_orm.column_property` to refer to joined-inheritance classes within a scalar subquery, which now works as intuitively as is feasible. ORM context, mapper, strategies now use ORMAdapter in all cases instead of straight ColumnAdapter; added some more parameters to ORMAdapter to make this possible. ORMAdapter now includes a "trace" enumeration that identifies the use path for the adapter and can aid in debugging. implement __slots__ for the ExternalTraversal hierarchy up to ORMAdapter. Within this change, we have to change the ClauseAdapter.wrap() method, which is only used in one polymorphic codepath, to use copy.copy() instead of `__dict__` access (apparently `__reduce_ex__` is implemented for objects with `__slots__`), and we also remove pickling ability, which should not be needed for adapters (this might have been needed for 1.3 and earlier in order for Query to be picklable, but none of that state is present within Query / select() / etc. anymore). Fixes: #8168 Change-Id: I3f6593eb02ab5e5964807c53a9fa4894c826d017 --- doc/build/changelog/unreleased_20/8168.rst | 27 +++ lib/sqlalchemy/orm/context.py | 59 ++++-- lib/sqlalchemy/orm/mapper.py | 50 ++++- lib/sqlalchemy/orm/strategies.py | 11 +- lib/sqlalchemy/orm/util.py | 120 ++++++++++-- lib/sqlalchemy/sql/util.py | 51 +++-- lib/sqlalchemy/sql/visitors.py | 13 +- test/orm/inheritance/test_assorted_poly.py | 35 ++-- tools/trace_orm_adapter.py | 217 +++++++++++++++++++++ 9 files changed, 508 insertions(+), 75 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/8168.rst create mode 100644 tools/trace_orm_adapter.py diff --git a/doc/build/changelog/unreleased_20/8168.rst b/doc/build/changelog/unreleased_20/8168.rst new file mode 100644 index 0000000000..4e15683b34 --- /dev/null +++ b/doc/build/changelog/unreleased_20/8168.rst @@ -0,0 +1,27 @@ +.. change:: + :tags: bug, orm + :tickets: 8168 + + Improved a fix first made in version 1.4 for :ticket:`8456` which scaled + back the usage of internal "polymorphic adapters", that are used to render + ORM queries when the :paramref:`_orm.Mapper.with_polymorphic` parameter is + used. These adapters, which are very complex and error prone, are now used + only in those cases where an explicit user-supplied subquery is used for + :paramref:`_orm.Mapper.with_polymorphic`, which includes only the use case + of concrete inheritance mappings that use the + :func:`_orm.polymorphic_union` helper, as well as the legacy use case of + using an aliased subquery for joined inheritance mappings, which is not + needed in modern use. + + For the most common case of joined inheritance mappings that use the + built-in polymorphic loading scheme, which includes those which make use of + the :paramref:`_orm.Mapper.polymorphic_load` parameter set to ``inline``, + polymorphic adapters are now no longer used. This has both a positive + performance impact on the construction of queries as well as a + substantial simplification of the internal query rendering process. + + The specific issue targeted was to allow a :func:`_orm.column_property` + to refer to joined-inheritance classes within a scalar subquery, which now + works as intuitively as is feasible. + + diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index b5b326bca2..b3c8e78b33 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -31,9 +31,11 @@ from .interfaces import ORMColumnsClauseRole from .path_registry import PathRegistry from .util import _entity_corresponds_to from .util import _ORMJoin +from .util import _TraceAdaptRole from .util import AliasedClass from .util import Bundle from .util import ORMAdapter +from .util import ORMStatementAdapter from .. import exc as sa_exc from .. import future from .. import inspect @@ -509,7 +511,15 @@ class ORMCompileState(AbstractORMCompileState): def _create_with_polymorphic_adapter(self, ext_info, selectable): """given MapperEntity or ORMColumnEntity, setup polymorphic loading - if appropriate + if called for by the Mapper. + + As of #8168 in 2.0.0b5, polymorphic adapters, which greatly increase + the complexity of the query creation process, are not used at all + except in the quasi-legacy cases of with_polymorphic referring to an + alias and/or subquery. This would apply to concrete polymorphic + loading, and joined inheritance where a subquery is + passed to with_polymorphic (which is completely unnecessary in modern + use). """ if ( @@ -521,7 +531,10 @@ class ORMCompileState(AbstractORMCompileState): self._mapper_loads_polymorphically_with( mp, ORMAdapter( - mp, mp._equivalent_columns, selectable=selectable + _TraceAdaptRole.WITH_POLYMORPHIC_ADAPTER, + mp, + equivalents=mp._equivalent_columns, + selectable=selectable, ), ) @@ -727,8 +740,11 @@ class ORMFromStatementCompileState(ORMCompileState): ) == "orm" ) - self._from_obj_alias = sql.util.ColumnAdapter( - self.statement, adapt_on_names=not statement_is_orm + + self._from_obj_alias = ORMStatementAdapter( + _TraceAdaptRole.ADAPT_FROM_STATEMENT, + self.statement, + adapt_on_names=not statement_is_orm, ) return self @@ -1068,6 +1084,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState): self._join_entities = () if self.compile_options._set_base_alias: + # legacy Query only self._set_select_from_alias() for memoized_entities in query._memoized_select_entities: @@ -1285,6 +1302,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState): return stmt def _set_select_from_alias(self): + """used only for legacy Query cases""" query = self.select_statement # query @@ -1297,6 +1315,8 @@ class ORMSelectCompileState(ORMCompileState, SelectState): self._from_obj_alias = adapter def _get_select_from_alias_from_obj(self, from_obj): + """used only for legacy Query cases""" + info = from_obj if "parententity" in info._annotations: @@ -1313,7 +1333,12 @@ class ORMSelectCompileState(ORMCompileState, SelectState): elif isinstance(info.selectable, sql.selectable.AliasedReturnsRows): equivs = self._all_equivs() - return sql_util.ColumnAdapter(info, equivs) + assert info is info.selectable + return ORMStatementAdapter( + _TraceAdaptRole.LEGACY_SELECT_FROM_ALIAS, + info.selectable, + equivalents=equivs, + ) else: return None @@ -1417,7 +1442,9 @@ class ORMSelectCompileState(ORMCompileState, SelectState): equivs = self._all_equivs() - self.compound_eager_adapter = sql_util.ColumnAdapter(inner, equivs) + self.compound_eager_adapter = ORMStatementAdapter( + _TraceAdaptRole.COMPOUND_EAGER_STATEMENT, inner, equivalents=equivs + ) statement = future.select( *([inner] + self.secondary_columns) # use_labels=self.labels @@ -2128,10 +2155,14 @@ class ORMSelectCompileState(ORMCompileState, SelectState): ) if need_adapter: + # if need_adapter is True, we are in a deprecated case and + # a warning has been emitted. assert right_mapper adapter = ORMAdapter( - inspect(right), equivalents=right_mapper._equivalent_columns + _TraceAdaptRole.DEPRECATED_JOIN_ADAPT_RIGHT_SIDE, + inspect(right), + equivalents=right_mapper._equivalent_columns, ) # if an alias() on the right side was generated, @@ -2142,11 +2173,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState): elif ( not r_info.is_clause_element and not right_is_aliased - and right_mapper.with_polymorphic - and isinstance( - right_mapper._with_polymorphic_selectable, - expression.AliasedReturnsRows, - ) + and right_mapper._has_aliased_polymorphic_fromclause ): # for the case where the target mapper has a with_polymorphic # set up, ensure an adapter is set up for criteria that works @@ -2159,9 +2186,11 @@ class ORMSelectCompileState(ORMCompileState, SelectState): # and similar self._mapper_loads_polymorphically_with( right_mapper, - sql_util.ColumnAdapter( - right_mapper.selectable, - right_mapper._equivalent_columns, + ORMAdapter( + _TraceAdaptRole.WITH_POLYMORPHIC_ADAPTER_RIGHT_JOIN, + right_mapper, + selectable=right_mapper.selectable, + equivalents=right_mapper._equivalent_columns, ), ) # if the onclause is a ClauseElement, adapt it with any diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index d15c882c40..7a75246212 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -103,6 +103,7 @@ if TYPE_CHECKING: from .properties import ColumnProperty from .relationships import RelationshipProperty from .state import InstanceState + from .util import ORMAdapter from ..engine import Row from ..engine import RowMapping from ..sql._typing import _ColumnExpressionArgument @@ -112,7 +113,6 @@ if TYPE_CHECKING: from ..sql.elements import ColumnElement from ..sql.schema import Column from ..sql.selectable import FromClause - from ..sql.util import ColumnAdapter from ..util import OrderedSet @@ -2440,6 +2440,20 @@ class Mapper( else: return None + @HasMemoized.memoized_attribute + def _has_aliased_polymorphic_fromclause(self): + """return True if with_polymorphic[1] is an aliased fromclause, + like a subquery. + + As of #8168, polymorphic adaption with ORMAdapter is used only + if this is present. + + """ + return self.with_polymorphic and isinstance( + self.with_polymorphic[1], + expression.AliasedReturnsRows, + ) + @HasMemoized.memoized_attribute def _should_select_with_poly_adapter(self): """determine if _MapperEntity or _ORMColumnEntity will need to use @@ -2456,6 +2470,10 @@ class Mapper( # polymorphic selectable, *or* if the base mapper has either of those, # we turn on the adaption thing. if not, we do *no* adaption. # + # (UPDATE for #8168: the above comment was not accurate, as we were + # still saying "do polymorphic" if we were using an auto-generated + # flattened JOIN for with_polymorphic.) + # # this splits the behavior among the "regular" joined inheritance # and single inheritance mappers, vs. the "weird / difficult" # concrete and joined inh mappings that use a with_polymorphic of @@ -2467,11 +2485,21 @@ class Mapper( # these tests actually adapt the polymorphic selectable (like, the # UNION or the SELECT subquery with JOIN in it) to be just the simple # subclass table. Hence even if we are a "plain" inheriting mapper - # but our base has a wpoly on it, we turn on adaption. + # but our base has a wpoly on it, we turn on adaption. This is a + # legacy case we should probably disable. + # + # + # UPDATE: simplified way more as of #8168. polymorphic adaption + # is turned off even if with_polymorphic is set, as long as there + # is no user-defined aliased selectable / subquery configured. + # this scales back the use of polymorphic adaption in practice + # to basically no cases except for concrete inheritance with a + # polymorphic base class. + # return ( - self.with_polymorphic + self._has_aliased_polymorphic_fromclause or self._requires_row_aliasing - or self.base_mapper.with_polymorphic + or (self.base_mapper._has_aliased_polymorphic_fromclause) or self.base_mapper._requires_row_aliasing ) @@ -2743,10 +2771,14 @@ class Mapper( ] @HasMemoized.memoized_attribute - def _polymorphic_adapter(self) -> Optional[sql_util.ColumnAdapter]: - if self.with_polymorphic: - return sql_util.ColumnAdapter( - self.selectable, equivalents=self._equivalent_columns + def _polymorphic_adapter(self) -> Optional[orm_util.ORMAdapter]: + if self._has_aliased_polymorphic_fromclause: + return orm_util.ORMAdapter( + orm_util._TraceAdaptRole.MAPPER_POLYMORPHIC_ADAPTER, + self, + selectable=self.selectable, + equivalents=self._equivalent_columns, + limit_on_entity=False, ) else: return None @@ -3213,7 +3245,7 @@ class Mapper( self, row: Optional[Union[Row[Any], RowMapping]], identity_token: Optional[Any] = None, - adapter: Optional[ColumnAdapter] = None, + adapter: Optional[ORMAdapter] = None, ) -> _IdentityKeyType[_O]: """Return an identity-map key for use in storing/retrieving an item from the identity map. diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index efa0dc680e..59031171da 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -2229,8 +2229,12 @@ class JoinedLoader(AbstractRelationshipLoader): if alias is not None: if isinstance(alias, str): alias = prop.target.alias(alias) - adapter = sql_util.ColumnAdapter( - alias, equivalents=prop.mapper._equivalent_columns + adapter = orm_util.ORMAdapter( + orm_util._TraceAdaptRole.JOINEDLOAD_USER_DEFINED_ALIAS, + prop.mapper, + selectable=alias, + equivalents=prop.mapper._equivalent_columns, + limit_on_entity=False, ) else: if path.contains( @@ -2240,6 +2244,7 @@ class JoinedLoader(AbstractRelationshipLoader): compile_state.attributes, "path_with_polymorphic" ) adapter = orm_util.ORMAdapter( + orm_util._TraceAdaptRole.JOINEDLOAD_PATH_WITH_POLYMORPHIC, with_poly_entity, equivalents=prop.mapper._equivalent_columns, ) @@ -2335,9 +2340,11 @@ class JoinedLoader(AbstractRelationshipLoader): to_adapt = self._gen_pooled_aliased_class(compile_state) to_adapt_insp = inspect(to_adapt) + clauses = to_adapt_insp._memo( ("joinedloader_ormadapter", self), orm_util.ORMAdapter, + orm_util._TraceAdaptRole.JOINEDLOAD_MEMOIZED_ADAPTER, to_adapt_insp, equivalents=self.mapper._equivalent_columns, adapt_required=True, diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 6ed8e22727..2a00d98fb4 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -8,9 +8,11 @@ from __future__ import annotations +import enum import re import types import typing +from typing import AbstractSet from typing import Any from typing import Callable from typing import cast @@ -441,25 +443,121 @@ def identity_key( raise sa_exc.ArgumentError("class or instance is required") +class _TraceAdaptRole(enum.Enum): + """Enumeration of all the use cases for ORMAdapter. + + ORMAdapter remains one of the most complicated aspects of the ORM, as it is + used for in-place adaption of column expressions to be applied to a SELECT, + replacing :class:`.Table` and other objects that are mapped to classes with + aliases of those tables in the case of joined eager loading, or in the case + of polymorphic loading as used with concrete mappings or other custom "with + polymorphic" parameters, with whole user-defined subqueries. The + enumerations provide an overview of all the use cases used by ORMAdapter, a + layer of formality as to the introduction of new ORMAdapter use cases (of + which none are anticipated), as well as a means to trace the origins of a + particular ORMAdapter within runtime debugging. + + SQLAlchemy 2.0 has greatly scaled back ORM features which relied heavily on + open-ended statement adaption, including the ``Query.with_polymorphic()`` + method and the ``Query.select_from_entity()`` methods, favoring + user-explicit aliasing schemes using the ``aliased()`` and + ``with_polymorphic()`` standalone constructs; these still use adaption, + however the adaption is applied in a narrower scope. + + """ + + # aliased() use that is used to adapt individual attributes at query + # construction time + ALIASED_INSP = enum.auto() + + # joinedload cases; typically adapt an ON clause of a relationship + # join + JOINEDLOAD_USER_DEFINED_ALIAS = enum.auto() + JOINEDLOAD_PATH_WITH_POLYMORPHIC = enum.auto() + JOINEDLOAD_MEMOIZED_ADAPTER = enum.auto() + + # polymorphic cases - these are complex ones that replace FROM + # clauses, replacing tables with subqueries + MAPPER_POLYMORPHIC_ADAPTER = enum.auto() + WITH_POLYMORPHIC_ADAPTER = enum.auto() + WITH_POLYMORPHIC_ADAPTER_RIGHT_JOIN = enum.auto() + DEPRECATED_JOIN_ADAPT_RIGHT_SIDE = enum.auto() + + # the from_statement() case, used only to adapt individual attributes + # from a given statement to local ORM attributes at result fetching + # time. assigned to ORMCompileState._from_obj_alias + ADAPT_FROM_STATEMENT = enum.auto() + + # the joinedload for queries that have LIMIT/OFFSET/DISTINCT case; + # the query is placed inside of a subquery with the LIMIT/OFFSET/etc., + # joinedloads are then placed on the outside. + # assigned to ORMCompileState.compound_eager_adapter + COMPOUND_EAGER_STATEMENT = enum.auto() + + # the legacy Query._set_select_from() case. + # this is needed for Query's set operations (i.e. UNION, etc. ) + # as well as "legacy from_self()", which while removed from 2.0 as + # public API, is used for the Query.count() method. this one + # still does full statement traversal + # assigned to ORMCompileState._from_obj_alias + LEGACY_SELECT_FROM_ALIAS = enum.auto() + + +class ORMStatementAdapter(sql_util.ColumnAdapter): + """ColumnAdapter which includes a role attribute.""" + + __slots__ = ("role",) + + def __init__( + self, + role: _TraceAdaptRole, + selectable: Selectable, + *, + equivalents: Optional[_EquivalentColumnMap] = None, + adapt_required: bool = False, + allow_label_resolve: bool = True, + anonymize_labels: bool = False, + adapt_on_names: bool = False, + adapt_from_selectables: Optional[AbstractSet[FromClause]] = None, + ): + self.role = role + super().__init__( + selectable, + equivalents=equivalents, + adapt_required=adapt_required, + allow_label_resolve=allow_label_resolve, + anonymize_labels=anonymize_labels, + adapt_on_names=adapt_on_names, + adapt_from_selectables=adapt_from_selectables, + ) + + class ORMAdapter(sql_util.ColumnAdapter): """ColumnAdapter subclass which excludes adaptation of entities from non-matching mappers. """ + __slots__ = ("role", "mapper", "is_aliased_class", "aliased_insp") + is_aliased_class: bool aliased_insp: Optional[AliasedInsp[Any]] def __init__( self, + role: _TraceAdaptRole, entity: _InternalEntityType[Any], + *, equivalents: Optional[_EquivalentColumnMap] = None, adapt_required: bool = False, allow_label_resolve: bool = True, anonymize_labels: bool = False, selectable: Optional[Selectable] = None, + limit_on_entity: bool = True, + adapt_on_names: bool = False, + adapt_from_selectables: Optional[AbstractSet[FromClause]] = None, ): - + self.role = role self.mapper = entity.mapper if selectable is None: selectable = entity.selectable @@ -470,21 +568,18 @@ class ORMAdapter(sql_util.ColumnAdapter): self.is_aliased_class = False self.aliased_insp = None - sql_util.ColumnAdapter.__init__( - self, + super().__init__( selectable, equivalents, adapt_required=adapt_required, allow_label_resolve=allow_label_resolve, anonymize_labels=anonymize_labels, - include_fn=self._include_fn, + include_fn=self._include_fn if limit_on_entity else None, + adapt_on_names=adapt_on_names, + adapt_from_selectables=adapt_from_selectables, ) def _include_fn(self, elem): - # TODO: we still have cases where we should return False here - # yet we are not able to reliably detect without false positives. - # see issue #8168 - entity = elem._annotations.get("parentmapper", None) return not entity or entity.isa(self.mapper) or self.mapper.isa(entity) @@ -774,7 +869,7 @@ class AliasedInsp( mapper: Mapper[_O] selectable: FromClause - _adapter: sql_util.ColumnAdapter + _adapter: ORMAdapter with_polymorphic_mappers: Sequence[Mapper[Any]] _with_polymorphic_entities: Sequence[AliasedInsp[Any]] @@ -840,8 +935,10 @@ class AliasedInsp( self._is_with_polymorphic = False self.with_polymorphic_mappers = [mapper] - self._adapter = sql_util.ColumnAdapter( - selectable, + self._adapter = ORMAdapter( + _TraceAdaptRole.ALIASED_INSP, + mapper, + selectable=selectable, equivalents=mapper._equivalent_columns, adapt_on_names=adapt_on_names, anonymize_labels=True, @@ -853,6 +950,7 @@ class AliasedInsp( for m in self.with_polymorphic_mappers if not adapt_on_names }, + limit_on_entity=False, ) if nest_adapters: diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index d162428ec3..bef4372ecb 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -12,6 +12,7 @@ from __future__ import annotations from collections import deque +import copy from itertools import chain import typing from typing import AbstractSet @@ -1064,6 +1065,16 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): """ + __slots__ = ( + "__traverse_options__", + "selectable", + "include_fn", + "exclude_fn", + "equivalents", + "adapt_on_names", + "adapt_from_selectables", + ) + def __init__( self, selectable: Selectable, @@ -1136,6 +1147,11 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): # TODO: cython candidate + if self.include_fn and not self.include_fn(col): # type: ignore + return None + elif self.exclude_fn and self.exclude_fn(col): # type: ignore + return None + if isinstance(col, FromClause) and not isinstance( col, functions.FunctionElement ): @@ -1173,6 +1189,7 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): # however the logic to check this moved here as of #7154 so that # it is made specific to SQL rewriting and not all column # correspondence + return None if "adapt_column" in col._annotations: @@ -1191,14 +1208,9 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): if TYPE_CHECKING: assert isinstance(col, KeyedColumnElement) - if self.include_fn and not self.include_fn(col): - return None - elif self.exclude_fn and self.exclude_fn(col): - return None - else: - return self._corresponding_column( # type: ignore - col, require_embedded=True - ) + return self._corresponding_column( # type: ignore + col, require_embedded=True + ) class _ColumnLookup(Protocol): @@ -1253,6 +1265,14 @@ class ColumnAdapter(ClauseAdapter): """ + __slots__ = ( + "columns", + "adapt_required", + "allow_label_resolve", + "_wrap", + "__weakref__", + ) + columns: _ColumnLookup def __init__( @@ -1267,8 +1287,7 @@ class ColumnAdapter(ClauseAdapter): anonymize_labels: bool = False, adapt_from_selectables: Optional[AbstractSet[FromClause]] = None, ): - ClauseAdapter.__init__( - self, + super().__init__( selectable, equivalents, include_fn=include_fn, @@ -1301,8 +1320,7 @@ class ColumnAdapter(ClauseAdapter): return self.columns[key] def wrap(self, adapter): - ac = self.__class__.__new__(self.__class__) - ac.__dict__.update(self.__dict__) + ac = copy.copy(self) ac._wrap = adapter ac.columns = util.WeakPopulateDict(ac._locate_col) # type: ignore if ac.include_fn or ac.exclude_fn: @@ -1391,15 +1409,6 @@ class ColumnAdapter(ClauseAdapter): return c - def __getstate__(self): - d = self.__dict__.copy() - del d["columns"] - return d - - def __setstate__(self, state): - self.__dict__.update(state) - self.columns = util.WeakPopulateDict(self._locate_col) # type: ignore - def _offset_or_limit_clause( element: Union[int, _ColumnExpressionArgument[int]], diff --git a/lib/sqlalchemy/sql/visitors.py b/lib/sqlalchemy/sql/visitors.py index 7371078443..b820cf9d17 100644 --- a/lib/sqlalchemy/sql/visitors.py +++ b/lib/sqlalchemy/sql/visitors.py @@ -656,7 +656,7 @@ class _TraverseTransformCallableType(Protocol[_ET]): _ExtT = TypeVar("_ExtT", bound="ExternalTraversal") -class ExternalTraversal: +class ExternalTraversal(util.MemoizedSlots): """Base class for visitor objects which can traverse externally using the :func:`.visitors.traverse` function. @@ -665,6 +665,8 @@ class ExternalTraversal: """ + __slots__ = ("_visitor_dict", "_next") + __traverse_options__: Dict[str, Any] = {} _next: Optional[ExternalTraversal] @@ -698,8 +700,9 @@ class ExternalTraversal: return traverse(obj, self.__traverse_options__, self._visitor_dict) - @util.memoized_property - def _visitor_dict(self) -> Dict[str, _TraverseCallableType[Any]]: + def _memoized_attr__visitor_dict( + self, + ) -> Dict[str, _TraverseCallableType[Any]]: visitors = {} for name in dir(self): @@ -737,6 +740,8 @@ class CloningExternalTraversal(ExternalTraversal): """ + __slots__ = () + def copy_and_process( self, list_: List[ExternallyTraversible] ) -> List[ExternallyTraversible]: @@ -773,6 +778,8 @@ class ReplacingExternalTraversal(CloningExternalTraversal): """ + __slots__ = () + def replace( self, elem: ExternallyTraversible ) -> Optional[ExternallyTraversible]: diff --git a/test/orm/inheritance/test_assorted_poly.py b/test/orm/inheritance/test_assorted_poly.py index 8ec36d2993..4bebc9b102 100644 --- a/test/orm/inheritance/test_assorted_poly.py +++ b/test/orm/inheritance/test_assorted_poly.py @@ -30,7 +30,6 @@ from sqlalchemy.testing import AssertsExecutionResults from sqlalchemy.testing import config from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures -from sqlalchemy.testing import skip_test from sqlalchemy.testing.fixtures import ComparableEntity from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.provision import normalize_sequence @@ -2411,7 +2410,7 @@ class Issue8168Test(AssertsCompiledSQL, fixtures.TestBase): def mapping(self, decl_base): Base = decl_base - def go(scenario, use_poly): + def go(scenario, use_poly, use_poly_on_retailer): class Customer(Base): __tablename__ = "customer" id = Column(Integer, primary_key=True) @@ -2469,7 +2468,12 @@ class Issue8168Test(AssertsCompiledSQL, fixtures.TestBase): .scalar_subquery() ) - __mapper_args__ = {"polymorphic_identity": "retailer"} + __mapper_args__ = { + "polymorphic_identity": "retailer", + "polymorphic_load": "inline" + if use_poly_on_retailer + else None, + } return Customer, Store, Retailer @@ -2477,8 +2481,13 @@ class Issue8168Test(AssertsCompiledSQL, fixtures.TestBase): @testing.variation("scenario", ["mapped_cls", "table", "table_alias"]) @testing.variation("use_poly", [True, False]) - def test_select_attr_only(self, scenario, use_poly, mapping): - Customer, Store, Retailer = mapping(scenario, use_poly) + @testing.variation("use_poly_on_retailer", [True, False]) + def test_select_attr_only( + self, scenario, use_poly, use_poly_on_retailer, mapping + ): + Customer, Store, Retailer = mapping( + scenario, use_poly, use_poly_on_retailer + ) if scenario.mapped_cls: self.assert_compile( @@ -2509,13 +2518,15 @@ class Issue8168Test(AssertsCompiledSQL, fixtures.TestBase): @testing.variation("scenario", ["mapped_cls", "table", "table_alias"]) @testing.variation("use_poly", [True, False]) - def test_select_cls(self, scenario, mapping, use_poly): - Customer, Store, Retailer = mapping(scenario, use_poly) + @testing.variation("use_poly_on_retailer", [True, False]) + def test_select_cls( + self, scenario, mapping, use_poly, use_poly_on_retailer + ): + Customer, Store, Retailer = mapping( + scenario, use_poly, use_poly_on_retailer + ) if scenario.mapped_cls: - # breaks for use_poly, but this is not totally unexpected - if use_poly: - skip_test("Case not working yet") self.assert_compile( select(Retailer), "SELECT (SELECT count(store.id) AS count_1 FROM customer " @@ -2525,10 +2536,6 @@ class Issue8168Test(AssertsCompiledSQL, fixtures.TestBase): "FROM customer JOIN retailer ON customer.id = retailer.id", ) elif scenario.table: - # TODO: breaks for use_poly, and this should not happen. - # selecting from the Table should be honoring that - if use_poly: - skip_test("Case not working yet") self.assert_compile( select(Retailer), "SELECT (SELECT count(store.id) AS count_1 FROM store " diff --git a/tools/trace_orm_adapter.py b/tools/trace_orm_adapter.py new file mode 100644 index 0000000000..42a23c9f72 --- /dev/null +++ b/tools/trace_orm_adapter.py @@ -0,0 +1,217 @@ +""" +Debug ORMAdapter calls within ORM runs. + +Demos:: + + python tools/trace_orm_adapter.py -m pytest \ + test/orm/inheritance/test_polymorphic_rel.py::PolymorphicAliasedJoinsTest::test_primary_eager_aliasing_joinedload + + python tools/trace_orm_adapter.py -m pytest \ + test/orm/test_eager_relations.py::LazyLoadOptSpecificityTest::test_pathed_joinedload_aliased_abs_bcs + + python tools/trace_orm_adapter.py my_test_script.py + + +The above two tests should spit out a ton of debug output. If a test or program +has no debug output at all, that's a good thing! it means ORMAdapter isn't +used for that case. + +You can then set a breakpoint at the end of any adapt step: + + python tools/trace_orm_adapter.py -d 10 -m pytest -s \ + test/orm/test_eager_relations.py::LazyLoadOptSpecificityTest::test_pathed_joinedload_aliased_abs_bcs + + +""" # noqa: E501 + +from __future__ import annotations + +import argparse +import contextlib +import contextvars +import sys +from typing import TYPE_CHECKING + +from sqlalchemy.orm import util + + +if TYPE_CHECKING: + from typing import Any + from typing import List + from typing import Optional + + from sqlalchemy.sql.elements import ColumnElement + + +class _ORMAdapterTrace: + def _locate_col( + self, col: ColumnElement[Any] + ) -> Optional[ColumnElement[Any]]: + with self._tracer("_locate_col") as tracer: + return tracer(super()._locate_col, col) + + def replace(self, col, _include_singleton_constants: bool = False): + with self._tracer("replace") as tracer: + return tracer(super().replace, col) + + _orm_adapter_trace_context = contextvars.ContextVar("_tracer") + + @contextlib.contextmanager + def _tracer(self, meth): + adapter = self + ctx = self._orm_adapter_trace_context.get( + {"stack": [], "last_depth": 0, "line_no": 0} + ) + self._orm_adapter_trace_context.set(ctx) + + stack: List[Any] = ctx["stack"] # type: ignore + last_depth = len(stack) + line_no: int = ctx["line_no"] # type: ignore + ctx["last_depth"] = last_depth + stack.append((adapter, meth)) + indent = " " * last_depth + + if hasattr(adapter, "mapper"): + adapter_desc = ( + f"{adapter.__class__.__name__}" + f"({adapter.role.name}, mapper={adapter.mapper})" + ) + else: + adapter_desc = f"{adapter.__class__.__name__}({adapter.role.name})" + + def tracer_fn(fn, arg): + nonlocal line_no + + line_no += 1 + + print(f"{indent} {line_no} {adapter_desc}", file=REAL_STDOUT) + sub_indent = " " * len(f"{line_no} ") + + print( + f"{indent}{sub_indent} -> " + f"{meth} {_orm_adapter_trace_print(arg)}", + file=REAL_STDOUT, + ) + ctx["line_no"] = line_no + ret = fn(arg) + + if DEBUG_ADAPT_STEP == line_no: + breakpoint() + + if ret is arg: + print(f"{indent} {line_no} <- same object", file=REAL_STDOUT) + else: + print( + f"{indent} {line_no} <- {_orm_adapter_trace_print(ret)}", + file=REAL_STDOUT, + ) + + if last_depth == 0: + print("", file=REAL_STDOUT) + return ret + + try: + yield tracer_fn + finally: + stack.pop(-1) + + +util.ORMAdapter.__bases__ = (_ORMAdapterTrace,) + util.ORMAdapter.__bases__ +util.ORMStatementAdapter.__bases__ = ( + _ORMAdapterTrace, +) + util.ORMStatementAdapter.__bases__ + + +def _orm_adapter_trace_print(obj): + if obj is None: + return "None" + + t_print = _orm_adapter_trace_printers.get(obj.__visit_name__, None) + if t_print: + return t_print(obj) + else: + return f"{obj!r}" + + +_orm_adapter_trace_printers = { + "table": lambda t: ( + f'Table("{t.name}", ' + f"entity={t._annotations.get('parentmapper', None)})" + ), + "column": lambda c: ( + f'Column("{c.name}", {_orm_adapter_trace_print(c.table)} ' + f"entity={c._annotations.get('parentmapper', None)})" + ), + "join": lambda j: ( + f"{j.__class__.__name__}({_orm_adapter_trace_print(j.left)}, " + f"{_orm_adapter_trace_print(j.right)})" + ), + "label": lambda l: f"Label({_orm_adapter_trace_print(l.element)})", +} + +DEBUG_ADAPT_STEP = None +REAL_STDOUT = sys.__stdout__ + + +def main(): + global DEBUG_ADAPT_STEP + + parser = argparse.ArgumentParser() + parser.add_argument( + "-d", "--debug", type=int, help="breakpoint at this adaptation step" + ) + parser.add_argument( + "-m", + "--module", + type=str, + help="import module name instead of running a script", + ) + parser.add_argument( + "args", metavar="N", type=str, nargs="*", help="additional arguments" + ) + + argparse_args = [] + sys_argv = list(sys.argv) + + progname = sys_argv.pop(0) + + # this is a little crazy, works at the moment for: + # module w args: + # python tools/trace_orm_adapter.py -m pytest test/orm/test_query.py -s + # script: + # python tools/trace_orm_adapter.py test3.py + has_module = False + while sys_argv: + arg = sys_argv.pop(0) + if arg in ("-m", "--module", "-d", "--debug"): + argparse_args.append(arg) + argparse_args.append(sys_argv.pop(0)) + has_module = arg in ("-m", "--module") + else: + if not has_module: + argparse_args.append(arg) + else: + sys_argv.insert(0, arg) + break + + options = parser.parse_args(argparse_args) + sys.argv = ["program.py"] + sys_argv + + if options.module == "pytest": + sys.argv.extend(["--capture", "sys"]) + + import runpy + + if options.debug: + DEBUG_ADAPT_STEP = options.debug + + if options.module: + runpy.run_module(options.module, run_name="__main__") + else: + progname = options.args[0] + + runpy.run_path(progname) + + +if __name__ == "__main__": + main() -- 2.47.2