From 93dc7ea1502c37793011b094447641361aff5aba Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 13 Nov 2022 11:49:43 -0500 Subject: don't invoke fromclause.c when creating an annotated The ``aliased()`` constructor calls upon ``__clause_element__()``, which internally annotates a ``FromClause``, like a subquery. This became expensive as ``AnnotatedFromClause`` has for many years called upon ``element.c`` so that the full ``.c`` collection is transferred to the Annotated. Taking this out proved to be challenging. A straight remove seemed to not break any tests except for the one that tested the exact condition. Nevertheless this seemed "spooky" so I instead moved the get of ``.c`` to be in a memoized proxy method. However, that then exposed a recursion issue related to loader_criteria; so the source of that behavior, which was an accidental behavioral artifact, is now made into an explcicit option that loader_criteria uses directly. The accidental behavioral artifact in question is still kind of strange since I was not able to fully trace out how it works, but the end result is that fixing the artifact to be "correct" causes loader_criteria, within the particular test for #7491, creates a select/ subquery structure with a cycle in it, so compilation fails with recursion overflow. The "solution" is to cause the artifact to occur in this case, which is that the ``AnnotatedFromClause`` will have a different ``.c`` collection than its element, which is a subquery. It's not totally clear how a cycle is generated when this is not done. This is commit one of two, which goes through some hoops to make essentially a one-line change. The next commit will rework ColumnCollection to optimize the corresponding_column() method significantly. Fixes: #8796 Change-Id: Id58ae6554db62139462c11a8be7313a3677456ad --- lib/sqlalchemy/sql/annotation.py | 17 ++++++++++++++++- lib/sqlalchemy/sql/selectable.py | 30 +++++++++++++++++++++++++----- lib/sqlalchemy/sql/util.py | 4 +++- 3 files changed, 44 insertions(+), 7 deletions(-) (limited to 'lib/sqlalchemy/sql') diff --git a/lib/sqlalchemy/sql/annotation.py b/lib/sqlalchemy/sql/annotation.py index 43ca84abb..3ce524447 100644 --- a/lib/sqlalchemy/sql/annotation.py +++ b/lib/sqlalchemy/sql/annotation.py @@ -421,6 +421,7 @@ def _deep_annotate( annotations: _AnnotationDict, exclude: Optional[Sequence[SupportsAnnotations]] = None, detect_subquery_cols: bool = False, + ind_cols_on_fromclause: bool = False, ) -> _SA: """Deep copy the given ClauseElement, annotating each element with the given annotations dictionary. @@ -435,6 +436,16 @@ def _deep_annotate( cloned_ids: Dict[int, SupportsAnnotations] = {} def clone(elem: SupportsAnnotations, **kw: Any) -> SupportsAnnotations: + + # ind_cols_on_fromclause means make sure an AnnotatedFromClause + # has its own .c collection independent of that which its proxying. + # this is used specifically by orm.LoaderCriteriaOption to break + # a reference cycle that it's otherwise prone to building, + # see test_relationship_criteria-> + # test_loader_criteria_subquery_w_same_entity. logic here was + # changed for #8796 and made explicit; previously it occurred + # by accident + kw["detect_subquery_cols"] = detect_subquery_cols id_ = id(elem) @@ -454,7 +465,11 @@ def _deep_annotate( newelem = elem._annotate(annotations) else: newelem = elem - newelem._copy_internals(clone=clone) + + newelem._copy_internals( + clone=clone, ind_cols_on_fromclause=ind_cols_on_fromclause + ) + cloned_ids[id_] = newelem return newelem diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 488dfe721..3d3aea3f2 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -6752,8 +6752,28 @@ TextAsFrom = TextualSelect class AnnotatedFromClause(Annotated): - def __init__(self, element, values): - # force FromClause to generate their internal - # collections into __dict__ - element.c - Annotated.__init__(self, element, values) + def _copy_internals(self, **kw): + super()._copy_internals(**kw) + if kw.get("ind_cols_on_fromclause", False): + ee = self._Annotated__element # type: ignore + + self.c = ee.__class__.c.fget(self) # type: ignore + + @util.ro_memoized_property + def c(self) -> ReadOnlyColumnCollection[str, KeyedColumnElement[Any]]: + """proxy the .c collection of the underlying FromClause. + + Originally implemented in 2008 as a simple load of the .c collection + when the annotated construct was created (see d3621ae961a), in modern + SQLAlchemy versions this can be expensive for statements constructed + with ORM aliases. So for #8796 SQLAlchemy 2.0 we instead proxy + it, which works just as well. + + Two different use cases seem to require the collection either copied + from the underlying one, or unique to this AnnotatedFromClause. + + See test_selectable->test_annotated_corresponding_column + + """ + ee = self._Annotated__element # type: ignore + return ee.c # type: ignore diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index 55c6a35f8..623a3f896 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -1197,7 +1197,9 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): elif self.exclude_fn and self.exclude_fn(col): return None else: - return self._corresponding_column(col, True) # type: ignore + return self._corresponding_column( # type: ignore + col, require_embedded=True + ) class _ColumnLookup(Protocol): -- cgit v1.2.1