Apply nested for joined eager m2o w/ GROUP BY, DISTINCT

Fixed issue where joined eager loading would fail to use the "nested" form
of the query when GROUP BY or DISTINCT were present if the eager joins
being added were many-to-ones, leading to additional columns in the columns
clause which would then cause errors.  The check for "nested" is tuned to
be enabled for these queries even for many-to-one joined eager loaders, and
the "only do nested if it's one to many" aspect is now localized to when
the query only has LIMIT or OFFSET added.

Fixes: #11226
Change-Id: I0b4b71cacfe1c6a25d0924e0954ceced1e399ca1

diff --git a/doc/build/changelog/unreleased_21/11226.rst b/doc/build/changelog/unreleased_21/11226.rst
new file mode 100644 (file)
index 0000000..11e871e
--- /dev/null
+++ b/doc/build/changelog/unreleased_21/11226.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 11226
+
+    Fixed issue where joined eager loading would fail to use the "nested" form
+    of the query when GROUP BY or DISTINCT were present if the eager joins
+    being added were many-to-ones, leading to additional columns in the columns
+    clause which would then cause errors.  The check for "nested" is tuned to
+    be enabled for these queries even for many-to-one joined eager loaders, and
+    the "only do nested if it's one to many" aspect is now localized to when
+    the query only has LIMIT or OFFSET added.

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index 15a9d2a7869906a687fee182b65b7540e7931d12..e906fcbc0f0d63052fdd379d22ab382908ab3f89 100644 (file)
--- a/lib/sqlalchemy/orm/context.py
+++ b/lib/sqlalchemy/orm/context.py
@@ -1405,11 +1405,7 @@ class _ORMSelectCompileState(_ORMCompileState, SelectState):
         if self.order_by is False:
             self.order_by = None
 
-        if (
-            self.multi_row_eager_loaders
-            and self.eager_adding_joins
-            and self._should_nest_selectable
-        ):
+        if self._should_nest_selectable:
             self.statement = self._compound_eager_statement()
         else:
             self.statement = self._simple_statement()
@@ -2431,9 +2427,19 @@ class _ORMSelectCompileState(_ORMCompileState, SelectState):
     @property
     def _should_nest_selectable(self):
         kwargs = self._select_args
+
+        if not self.eager_adding_joins:
+            return False
+
         return (
-            kwargs.get("limit_clause") is not None
-            or kwargs.get("offset_clause") is not None
+            (
+                kwargs.get("limit_clause") is not None
+                and self.multi_row_eager_loaders
+            )
+            or (
+                kwargs.get("offset_clause") is not None
+                and self.multi_row_eager_loaders
+            )
             or kwargs.get("distinct", False)
             or kwargs.get("distinct_on", ())
             or kwargs.get("group_by", False)

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 8af9c020382f7c8eec418e64837654e606874f95..cd0d97598f29382786c131b22e13590094d63c83 100644 (file)
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -2448,10 +2448,7 @@ class _JoinedLoader(_AbstractRelationshipLoader):
         # whether or not the Query will wrap the selectable in a subquery,
         # and then attach eager load joins to that (i.e., in the case of
         # LIMIT/OFFSET etc.)
-        should_nest_selectable = (
-            compile_state.multi_row_eager_loaders
-            and compile_state._should_nest_selectable
-        )
+        should_nest_selectable = compile_state._should_nest_selectable
 
         query_entity_key = None
 

diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py
index 7e0eca62c6532f141f2942e48e09d2cb8152aa4f..80178582c6e00e1e9bab5191a4a90682a7a52428 100644 (file)
--- a/test/orm/test_eager_relations.py
+++ b/test/orm/test_eager_relations.py
@@ -3133,6 +3133,89 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL):
             ],
         )
 
+    @testing.fixture
+    def issue_11226_fixture(self):
+        users, items, order_items, Order, Item, User, orders = (
+            self.tables.users,
+            self.tables.items,
+            self.tables.order_items,
+            self.classes.Order,
+            self.classes.Item,
+            self.classes.User,
+            self.tables.orders,
+        )
+
+        self.mapper_registry.map_imperatively(
+            User,
+            users,
+        )
+        self.mapper_registry.map_imperatively(
+            Order,
+            orders,
+            properties=dict(
+                items=relationship(
+                    Item, secondary=order_items, order_by=items.c.id
+                ),
+                user=relationship(User),
+            ),
+        )
+        self.mapper_registry.map_imperatively(Item, items)
+
+    def test_nested_for_group_by(self, issue_11226_fixture):
+        """test issue #11226"""
+
+        Order, Item = self.classes("Order", "Item")
+
+        stmt = (
+            select(Order, func.count(Item.id))
+            .join(Order.items)
+            .group_by(Order.id)
+            .options(joinedload(Order.user))
+        )
+
+        # the query has a many-to-one joinedload, but also a GROUP BY.
+        # eager loading needs to use nested form so that the eager joins
+        # can be added to the outside of the GROUP BY query.
+        # change #11226 liberalizes the conditions where we do nested form
+        # to include non-multi-row eager loads, when the columns list is
+        # otherwise sensitive to more columns being added.
+        self.assert_compile(
+            stmt,
+            "SELECT anon_1.id, anon_1.user_id, anon_1.address_id, "
+            "anon_1.description, anon_1.isopen, anon_1.count_1, "
+            "users_1.id AS id_1, users_1.name "
+            "FROM (SELECT orders.id AS id, orders.user_id AS user_id, "
+            "orders.address_id AS address_id, "
+            "orders.description AS description, orders.isopen AS isopen, "
+            "count(items.id) AS count_1 "
+            "FROM orders "
+            "JOIN order_items AS order_items_1 "
+            "ON orders.id = order_items_1.order_id "
+            "JOIN items ON items.id = order_items_1.item_id "
+            "GROUP BY orders.id) "
+            "AS anon_1 "
+            "LEFT OUTER JOIN users AS users_1 ON users_1.id = anon_1.user_id",
+        )
+
+    def test_nested_for_distinct(self, issue_11226_fixture):
+        """test issue #11226"""
+
+        Order, Item = self.classes("Order", "Item")
+
+        stmt = select(Order).distinct().options(joinedload(Order.user))
+
+        self.assert_compile(
+            stmt,
+            "SELECT anon_1.id, anon_1.user_id, anon_1.address_id, "
+            "anon_1.description, anon_1.isopen, "
+            "users_1.id AS id_1, users_1.name "
+            "FROM (SELECT DISTINCT orders.id AS id, "
+            "orders.user_id AS user_id, orders.address_id AS address_id, "
+            "orders.description AS description, orders.isopen AS isopen "
+            "FROM orders) AS anon_1 "
+            "LEFT OUTER JOIN users AS users_1 ON users_1.id = anon_1.user_id",
+        )
+
 
 class SelectUniqueTest(_fixtures.FixtureTest):
     run_inserts = "once"