undefer_group() from relationship path has no effect

[Mike Bayer]

    def test_undefer_group_from_relationship_joinedload(self):
        users, Order, User, orders = \
            (self.tables.users,
             self.classes.Order,
             self.classes.User,
             self.tables.orders)

        mapper(User, users, properties=dict(
            orders=relationship(Order, order_by=orders.c.id)))
        mapper(
            Order, orders, properties=util.OrderedDict([
                ('userident', deferred(orders.c.user_id, group='primary')),
                ('description', deferred(orders.c.description,
                 group='primary')),
                ('opened', deferred(orders.c.isopen, group='secondary'))
            ])
        )

        sess = create_session()
        q = sess.query(User).filter(User.id == 7).options(
            joinedload(User.orders).undefer_group('primary')
        )

        def go():
            result = q.all()
            o2 = result[0].orders[1]
            eq_(o2.opened, 1)
            eq_(o2.userident, 7)
            eq_(o2.description, 'order 3')
        self.assert_sql_count(testing.db, go, 1)

[Mike Bayer]

it works with a lazyload, however:

    def test_undefer_group_from_relationship_lazyload(self):
        users, Order, User, orders = \
            (self.tables.users,
             self.classes.Order,
             self.classes.User,
             self.tables.orders)

        mapper(User, users, properties=dict(
            orders=relationship(Order, order_by=orders.c.id)))
        mapper(
            Order, orders, properties=util.OrderedDict([
                ('userident', deferred(orders.c.user_id, group='primary')),
                ('description', deferred(orders.c.description,
                 group='primary')),
                ('opened', deferred(orders.c.isopen, group='primary'))
            ])
        )

        sess = create_session()
        q = sess.query(User).filter(User.id == 7).options(
            defaultload(User.orders).undefer_group('primary')
        )

        def go():
            result = q.all()
            o2 = result[0].orders[1]
            eq_(o2.opened, 1)
            eq_(o2.userident, 7)
            eq_(o2.description, 'order 3')
        self.assert_sql_count(testing.db, go, 2)

[Mike Bayer]

see also #4049

[Mike Bayer]

we can confirm this bug goes back to the "quick_populators" thing introduced in 1.0. This patch against 1.0.18 does not break any tests and illustrates the incorrect code, and asserts all the conditions that illustrate a. it's wrong b. why it "works" except in this as yet uncovered undefer_group() case:

diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 82cd007..1646ba2 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -311,9 +311,22 @@ def _instance_processor(
                 populators["expire"].append((prop.key, False))
             else:
                 if adapter:
-                    col = adapter.columns[col]
+                    # we should not use adapter here, because a member
+                    # of the cached quick_populators has already applied
+                    # the adapter to the column.
+
+                    # we can confirm this here:
+                    adapter.columns[prop.columns[0]] is col # already adapted!
+
+                    # confirm the original column gives us a getter..
+                    orig_col = col
+                    orig_getter = result._getter(col, False)
+                    col = adapter.columns[col] #  this will often be None
+                                               #  because 'col' is already the
+                                               #  adapted
                 getter = result._getter(col, False)
                 if getter:
+                    # we won't be here because col is None
                     populators["quick"].append((prop.key, getter))
                 else:
                     # fall back to the ColumnProperty itself, which
@@ -321,6 +334,31 @@ def _instance_processor(
                     # to see if one fits
                     prop.create_row_processor(
                         context, path, mapper, result, adapter, populators)
+
+                    # let's look at the getter we got by going through
+                    # the row_processor again.  In all cases except
+                    # the special undefer_group case, the correct strategy
+                    # is used here via interfaces.MapperProperty.create_row_processor.
+                    # only undefer_group and a few others has a special short-circuit
+                    # in setup_query that bypasses this mechanism
+                    if adapter:
+                        canary = list(range(50))
+
+                        # lets get that new getter()
+                        new_getter = populators['quick'][-1][1]
+                        assert orig_getter
+                        assert new_getter
+
+                        # lets assert that the col we got above is either
+                        # None, or the same column
+                        assert col is None or col is orig_col or (
+                            col.__class__.__name__ == 'Label' and
+                            orig_col in col._cloned_set
+                        )
+
+                        # assert the new getter does the exact same thing
+                        # as the old getter
+                        assert orig_getter(canary) == new_getter(canary)
         else:
             prop.create_row_processor(
                 context, path, mapper, result, adapter, populators)

[Mike Bayer]

disproven on that already:

diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 82cd007..e8500d0 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -311,9 +311,13 @@ def _instance_processor(
                 populators["expire"].append((prop.key, False))
             else:
                 if adapter:
+                    orig_col = col
                     col = adapter.columns[col]
                 getter = result._getter(col, False)
                 if getter:
+                    if adapter and orig_col is not col:
+                        import pdb
+                        pdb.set_trace()
                     populators["quick"].append((prop.key, getter))
                 else:
                     # fall back to the ColumnProperty itself, which

test/orm/test_eager_relations.py::CorrelatedSubqueryTest::test_labeled_on_date_alias 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB set_trace (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /home/classic/tmp/sa1018/lib/sqlalchemy/orm/loading.py(321)_instance_processor()
-> populators["quick"].append((prop.key, getter))
(Pdb) orig_col
Column('name', String(length=50), table=<users>)
(Pdb) col
Column('users_name', String(length=50), table=<%(140704382340624 anon)s>)
(Pdb) prop.key
u'name'
(Pdb) quick_populators
{<ColumnProperty at 0x7ff84abbf410; id>: Column('id', Integer(), table=<users>, primary_key=True, nullable=False), <ColumnProperty at 0x7ff84abbf140; name>: Column('name', String(length=50), table=<users>)}

in this case, there's an adapter, the ColumnLoader did not adapt the column into the populators dict. this is in the case of a first() load where the additional subquery is applied.

So we just need to check for None here and use orig col if so.

[Mike Bayer]

here's a gerrit that addresses labeled expressions as well and by saving the row_processor() codepath we also bumped about 20% off of aaa_profiling/test_orm test_fetch_results

[Mike Bayer]

Ensure col is not None when retrieving quick populators

Fixed bug where an :func:.undefer_group option would not be recognized if it extended from a relationship that was loading using joined eager loading.

In particular we need to double check the column both in terms of the given "adapter" as well as without applying the "adapter" when searching for the column in the result.

As we now avoid redoing the row processor step we also improve on callcounts in joined eager loading.

Change-Id: I0f48766f12f7299f4626ff41a00bf1f5bfca5f3b Fixes: #4048

→ <<cset 827b495b8bc1>>

[Mike Bayer]

Ensure col is not None when retrieving quick populators

Fixed bug where an :func:.undefer_group option would not be recognized if it extended from a relationship that was loading using joined eager loading.

In particular we need to double check the column both in terms of the given "adapter" as well as without applying the "adapter" when searching for the column in the result.

As we now avoid redoing the row processor step we also improve on callcounts in joined eager loading.

Change-Id: I0f48766f12f7299f4626ff41a00bf1f5bfca5f3b Fixes: #4048 (cherry picked from commit eee9dfd4514801f0c67f71632fc722731171479b)

→ <<cset 3b5840f16a27>>