Re: [Sqlalchemy-tickets] [sqlalchemy] #2948: better replacement of columns w bound values on "local" side for lazy load (was: clarify behavior of explicit primaryjoin wrt backref, including importance of establishing both sides for unusual cases)

[sqlalchemy <mi...@zz...>]

#2948: better replacement of columns w bound values on "local" side for lazy load
------------------------------+-------------------------------
      Reporter:  mkadin       |      Owner:  zzzeek
          Type:  defect       |     Status:  new
      Priority:  high         |  Milestone:  0.9.3
     Component:  orm          |   Severity:  major - 1-3 hours
    Resolution:               |   Keywords:
Progress State:  in progress  |
------------------------------+-------------------------------
Changes (by zzzeek):

 * severity:  minor - half an hour => major - 1-3 hours
 * component:  documentation => orm
 * priority:  medium => high
 * milestone:  0.9.xx => 0.9.3
 * status_field:  in queue => in progress


Comment:

 change of plans, sorry.

 this can be detected and we can produce a more "correct" condition here,
 when we add a rule that all local side columns become bound values
 unconditionally.  its possible that older versions worked this way, will
 need to check.

 Sorry, the suggestion that lazyload render a JOIN threw me off here.  The
 bug is more about replacement of bound parameters.

 Patch:

 {{{
 #!diff
 diff --git a/lib/sqlalchemy/orm/relationships.py
 b/lib/sqlalchemy/orm/relationships.py
 index 62d4d6b..e21f8f4 100644
 --- a/lib/sqlalchemy/orm/relationships.py
 +++ b/lib/sqlalchemy/orm/relationships.py
 @@ -2479,6 +2479,7 @@ class JoinCondition(object):
          binds = util.column_dict()
          lookup = util.column_dict()
          equated_columns = util.column_dict()
 +        being_replaced = set()

          if reverse_direction and self.secondaryjoin is None:
              for l, r in self.local_remote_pairs:
 @@ -2486,16 +2487,30 @@ class JoinCondition(object):
                  _list.append((r, l))
                  equated_columns[l] = r
          else:
 +            # replace all "local side" columns, which is
 +            # anything that isn't marked "remote"
 +            being_replaced.update(self.local_columns)
              for l, r in self.local_remote_pairs:
                  _list = lookup.setdefault(l, [])
                  _list.append((l, r))
                  equated_columns[r] = l

          def col_to_bind(col):
 -            if col in lookup:
 -                for tobind, equated in lookup[col]:
 -                    if equated in binds:
 -                        return None
 +            if col in being_replaced or col in lookup:
 +                if col in lookup:
 +                    for tobind, equated in lookup[col]:
 +                        if equated in binds:
 +                            return None
 +                else:
 +                    assert not reverse_direction
 +                    # would like to warn here, but still some cases
 +                    # where the other column is still table-bound,
 +                    # so cant quite detect that here.
 +                    #util.warn(
 +                    #      "Relationship %s creating a fixed condition by
 "
 +                    #      "comparing local column %s to a constant; "
 +                    #      "consider revising this primaryjoin for
 greater "
 +                    #      "efficiency." % (self.prop, col))
                  if col not in binds:
                      binds[col] = sql.bindparam(
                          None, None, type_=col.type, unique=True)
 }}}

--
Ticket URL: <http://www.sqlalchemy.org/trac/ticket/2948#comment:3>
sqlalchemy <http://www.sqlalchemy.org/>
The Database Toolkit for Python