positional result column logic failing

[Mike Bayer]

Havent fully tracked this down yet, suspect the query wrapping in mssql for LIMIT/OFFSET is causing this to happen.

very heisenbuggy, set PYTHONHASHEED=random and run lots of times against mssql:

import sqlalchemy as sa
import sqlalchemy.orm as saorm
from sqlalchemy.ext.declarative import declarative_base

#engine = sa.create_engine('mssql+pymssql://badams:password@192.168.56.101:1443/testdb', echo=True)

engine = sa.create_engine('mssql+pymssql://scott:tiger@192.168.122.232:1213/test', echo=True)

session = saorm.sessionmaker(bind=engine)()

Base = declarative_base()

class Person(Base):
    __tablename__ = 'people'
    id = sa.Column(sa.Integer, primary_key=True)
    name = sa.Column(sa.String)

Base.metadata.create_all(engine)

session.query(Person).delete()

session.add(Person(name='foo'))
session.add(Person(name='bar'))

session.commit()

results = session.query(
    Person.name.label('person'),
).add_entity(
    Person
).order_by(
    Person.name
)

print results.count()
print results.limit(1).offset(1).all()

the stack is a lie; to really make this fail we have to raise here (and this is another issue for 1.1, why aren't we raising here? )

--- a/lib/sqlalchemy/engine/result.py
+++ b/lib/sqlalchemy/engine/result.py
@@ -420,7 +427,8 @@ class ResultMetaData(object):
         if key in self._keymap:
             processor, obj, index = self._keymap[key]
         else:
-            ret = self._key_fallback(key, False)
+            # MARKMARK
+            ret = self._key_fallback(key) #, False)
             if ret is None:
                 return None
             processor, obj, index = ret

so ultimately the compiler._result_columns is wrong. on a bad run its:

[

('person', 'person', (Column('name', String(), table=<people>), 'person', 'person'), String()), 
('people_id', 'people_id', (Column('id', Integer(), table=<people>, primary_key=True, nullable=False), 'people_id', 'people_id'), Integer()), ('people_name', 'people_name', (Column('name', String(), table=<people>), 'people_name', 'people_name'), String())

]

on a good run it's:

[

('person', 'person', (<sqlalchemy.sql.elements.Label object at 0x7f46cc3dcf10>, 'person', 'person'), String()), 
('people_id', 'people_id', (Column('id', Integer(), table=<people>, primary_key=True, nullable=False), 'people_id', 'people_id'), Integer()), ('people_name', 'people_name', (Column('name', String(), table=<people>), 'people_name', 'people_name'), String())
]

so our label is occasionally getting whacked from the result list, and its a dictionary ordering issue. the LIMIT wrapping in mssql/base.py -> visit_select seems like it may be significant also.

[Mike Bayer]

here's the bug:

diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py
index 722beb1..c13aac4 100644
--- a/lib/sqlalchemy/sql/compiler.py
+++ b/lib/sqlalchemy/sql/compiler.py
@@ -1591,6 +1591,11 @@ class SQLCompiler(Compiled):
             # if this select is a compiler-generated wrapper,
             # rewrite the targeted columns in the result map
             wrapped_inner_columns = set(select_wraps_for.inner_columns)
+            for outer in select.inner_columns:
+                if len(outer.proxy_set.intersection(wrapped_inner_columns)) != 1:
+                    print outer.proxy_set.intersection(wrapped_inner_columns)
+                    import pdb
+                    pdb.set_trace()
             translate = dict(
                 (outer, inner.pop()) for outer, inner in [
                     (

we're pulling from an intersection using pop() but that's not deterministic. on a good run the intersection is:

set([<sqlalchemy.sql.elements.Label object at 0x7f4256c51f10>, Column('name', String(), table=<people>)])

on a bad run, surprise, it's:

set([Column('name', String(), table=<people>), <sqlalchemy.sql.elements.Label object at 0x7f648f3da850>])

this logic is a lot like corresponding_column, but like that function, when the intersection is ambiguous we need to look more closely at the field of correspondence. and really im not sure why we aren't matching these columns positionally.

[Mike Bayer]

the most straightforward fix would be:

diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py
index 722beb1..e9f1f71 100644
--- a/lib/sqlalchemy/sql/compiler.py
+++ b/lib/sqlalchemy/sql/compiler.py
@@ -1590,14 +1590,9 @@ class SQLCompiler(Compiled):
         if populate_result_map and select_wraps_for is not None:
             # if this select is a compiler-generated wrapper,
             # rewrite the targeted columns in the result map
-            wrapped_inner_columns = set(select_wraps_for.inner_columns)
             translate = dict(
-                (outer, inner.pop()) for outer, inner in [
-                    (
-                        outer,
-                        outer.proxy_set.intersection(wrapped_inner_columns))
-                    for outer in select.inner_columns
-                ] if inner
+                (outer, select_wraps_for.corresponding_column(outer))
+                for outer in select.inner_columns
             )
             self._result_columns = [
                 (key, name, tuple(translate.get(o, o) for o in obj), type_)

however this is slower, and for some reason a bunch of tests fail in the mssql area that are testing this. need to see if the tests are valid or otherwise why plain corresponding column isn't sufficient. it works for the test here.

[Mike Bayer]

see #3658

[Mike Bayer]

• reworked the way the "select_wraps_for" expression is handled within visit_select(); this attribute was added in the 1.0 series to accommodate the subquery wrapping behavior of SQL Server and Oracle while also working with positional column targeting and no longer relying upon "key fallback" in order to target columns in such a statement. The IBM DB2 third-party dialect also has this use case, but its implementation is using regular expressions to rewrite the textual SELECT only and does not make use of a "wrapped" select at this time. The logic no longer attempts to reconcile proxy set collections as this was not deterministic, and instead assumes that the select() and the wrapper select() match their columns postionally, at least for the column positions they have in common, so it is now very simple and safe. fixes #3657.
• as a side effect of #3657 it was also revealed that the strategy of calling upon a ResultProxy._getter was not correctly calling into NoSuchColumnError when an expected column was not present, and instead returned None up to loading.instances() to produce NoneType failures; added a raiseerr argument to _getter() which is called when we aren't expecting None, fixes #3658.

→ <<cset 8ad968f33100>>

[Mike Bayer]

• reworked the way the "select_wraps_for" expression is handled within visit_select(); this attribute was added in the 1.0 series to accommodate the subquery wrapping behavior of SQL Server and Oracle while also working with positional column targeting and no longer relying upon "key fallback" in order to target columns in such a statement. The IBM DB2 third-party dialect also has this use case, but its implementation is using regular expressions to rewrite the textual SELECT only and does not make use of a "wrapped" select at this time. The logic no longer attempts to reconcile proxy set collections as this was not deterministic, and instead assumes that the select() and the wrapper select() match their columns postionally, at least for the column positions they have in common, so it is now very simple and safe. fixes #3657.
• as a side effect of #3657 it was also revealed that the strategy of calling upon a ResultProxy._getter was not correctly calling into NoSuchColumnError when an expected column was not present, and instead returned None up to loading.instances() to produce NoneType failures; added a raiseerr argument to _getter() which is called when we aren't expecting None, fixes #3658.

(cherry picked from commit 8ad968f33100baeb3b13c7e0b724b6b79ab4277f)

→ <<cset cf818984ab52>>

[Mike Bayer]

• additional adjustment to the fix made in 8ad968f33100baeb3b13c7e0b724b6b79ab4277f for ref #3657. The Oracle dialect makes more use of the "select_wraps_for" feature than SQL server because Oracle doesn't have "TOP" for a limit-only select, so tests are showing more happening here. In the case where the select() has some dupe columns, these are deduped from the .c collection so a positional match between the wrapper and original can't use .inner_columns, because these collections wont match. Using _columns_plus_names instead which is the deduped collection that determines the SELECT display, which definitely have to match up.

→ <<cset aa9ce3f521f2>>

[Mike Bayer]

• additional adjustment to the fix made in 8ad968f33100baeb3b13c7e0b724b6b79ab4277f for ref #3657. The Oracle dialect makes more use of the "select_wraps_for" feature than SQL server because Oracle doesn't have "TOP" for a limit-only select, so tests are showing more happening here. In the case where the select() has some dupe columns, these are deduped from the .c collection so a positional match between the wrapper and original can't use .inner_columns, because these collections wont match. Using _columns_plus_names instead which is the deduped collection that determines the SELECT display, which definitely have to match up.

(cherry picked from commit aa9ce3f521f254da9879ede011e520ec35b8270e)

→ <<cset eff7b4f29d2a>>