PySequence_Check failes on C implementation of ResultProxy

[Former user]

Just try this code with enabled or disabled extensions:

import csv
from StringIO import StringIO #for something filelike

from sqlalchemy import *


db = create_engine('sqlite:///tutorial.db')


metadata = MetaData(db)
users = Table('users', metadata,
    Column('user_id', Integer, primary_key=True),
    Column('name', String(40)),
    Column('age', Integer),
    Column('password', String),
)

users.create()


i = users.insert()
i.execute(name='Mary', age=30, password='secret')


s = users.select()
rs = s.execute()

row = rs.fetchone()


filelike = StringIO()
writer = csv.writer(filelike)
writer.writerow(row) #failes HERE!

_csv.c uses PySequence_Check which failes on ResultProxy made in C. I suppose the problem is here - no sq_item function provided, because PySequence_Check looks like this

int
PySequence_Check(PyObject *s)
{
        if (s && PyInstance_Check(s))
                return PyObject_HasAttrString(s, "__getitem__");
        if (PyObject_IsInstance(s, (PyObject *)&PyDict_Type))
                return 0;
        return s != NULL && s->ob_type->tp_as_sequence &&
                s->ob_type->tp_as_sequence->sq_item != NULL;
}

A simple patch helps:

--- SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c    2010-07-15 22:35:09.000000000 +0600
+++ SQLAlchemy-0.6.3_new/lib/sqlalchemy/cextension/resultproxy.c        2010-08-09 14:00:45.000000000 +0600
@@ -502,11 +502,12 @@
     {NULL}  /* Sentinel */
 };

+
 static PySequenceMethods BaseRowProxy_as_sequence = {
     (lenfunc)BaseRowProxy_length,   /* sq_length */
     0,                              /* sq_concat */
     0,                              /* sq_repeat */
-    0,                              /* sq_item */
+    BaseRowProxy_subscript,         /* sq_item */
     0,                              /* sq_slice */
     0,                              /* sq_ass_item */
     0,                              /* sq_ass_slice */

As a workaround I use row.values() for now...

[Former user]

(original author: ged) Well, the patch you suggested is not acceptable as is, because BaseRowProxy_subscript has not the correct signature for use as a sq_item function. It is not actually called in your use case (or ever -- I can't find any example of code triggering that code path directly, hence I didn't implement it in the extension).

For reference, operator.isSequenceType is the pure python equivalent of PySequence_Check. However it is deprecated in Python 2.6 and is not exactly equivalent to its suggested replacement: isinstance(obj, collections.Sequence), and it is unclear to me what exactly is necessary for a C extension to "pass" that check. Do you have any information on this?

[Former user]

Replying to ged:

BaseRowProxy_subscript has not the correct signature for use as a sq_item function. I guess so, didn't do any checks because it compiled whithout any warning. It is not actually called in your use case. Hmmm... To check if it's called I added printf in BaseRowProxy_subscript and I saw that message. You can check that easily - test case exists.

For reference, operator.isSequenceType is the pure python equivalent of PySequence_Check. However it is deprecated in Python 2.6 and is not exactly equivalent to its suggested replacement: isinstance(obj, collections.Sequence), and it is unclear to me what exactly is necessary for a C extension to "pass" that check. Do you have any information on this? I'll digg it during weekend!

Thanks for your response.

[Former user]

Hmm... Rechecked this issue.

Putting a dummy pointer in BaseRowProxy_as_sequence as sq_item makes test case pass, but that function is never called.

[Mike Bayer]

can someone chime in on what the status of this ticket is ? do we think that C extensions turned on by default in 0.7 is feasable ?

[Former user]

Did a little more digging. * PySequence_Check is passed if BaseRowProxy_as_sequence sq_item slot is not NULL.

• sq_item is never called because interpreter chooses mp_subscript over sq_item. Some info is here.

• Simple stub like

  #!python --- SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c 2010-07-15 22:35:09.000000000 +0600 +++ /tmp/SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c 2010-09-17 17:13:07.000000000 +0600 @@ -327,6 +327,12 @@ }

  static PyObject * +BaseRowProxy_getitem(PyObject self, Py_ssize_t i) +{ + return BaseRowProxy_subscript((BaseRowProxy)self, PyInt_FromSsize_t(i)); +} + +static PyObject * BaseRowProxy_getattro(BaseRowProxy self, PyObject name) { PyObject tmp; @@ -506,7 +512,7 @@ (lenfunc)BaseRowProxy_length, / sq_length / 0, / sq_concat / 0, / sq_repeat / - 0, / sq_item / + (ssizeargfunc)BaseRowProxy_getitem, / sq_item / 0, / sq_slice / 0, / sq_ass_item / 0, / sq_ass_slice */

does well: * sq_item is filled, but never called * if (for some reason it's called) - it works (tested by setting mp_subscript to 0).

Replying to zzzeek:

can someone chime in on what the status of this ticket is ? do we think that C extensions turned on by default in 0.7 is feasable ? We use cextensions in production. Everything seems okay. Except for this ticket :)

[Mike Bayer]

If we can't wake up ged on this ticket, it will definitely be pushed out to 0.6.xx very soon by the time 0.6.5 goes out, and will likely end up in blue sky. Definitely am not turning on C extensions by default in 0.7 until this one gets a real test + resolution.

[Former user]

If we can't wake up ged on this ticket If so, let's make some loud noises to wake him up. Docs say about type checking here.

To pass new style type checking (someone would definitely check that way some day) we should register our class as sequence type (thanks to dash at #python on freenode). {{ diff -ru /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c ./lib/sqlalchemy/cextension/resultproxy.c --- /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c 2010-07-15 22:35:09.000000000 +0600 +++ ./lib/sqlalchemy/cextension/resultproxy.c 2010-10-21 11:28:45.000000000 +0600 @@ -327,6 +327,12 @@ }

static PyObject * +BaseRowProxy_getitem(PyObject self, Py_ssize_t i) +{ + return BaseRowProxy_subscript((BaseRowProxy)self, PyInt_FromSsize_t(i)); +} + +static PyObject * BaseRowProxy_getattro(BaseRowProxy self, PyObject name) { PyObject tmp; @@ -506,7 +512,7 @@ (lenfunc)BaseRowProxy_length, / sq_length / 0, / sq_concat / 0, / sq_repeat / - 0, / sq_item / + (ssizeargfunc)BaseRowProxy_getitem, / sq_item / 0, / sq_slice / 0, / sq_ass_item / 0, / sq_ass_slice */ diff -ru /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/engine/base.py ./lib/sqlalchemy/engine/base.py --- /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/engine/base.py 2010-07-15 22:35:10.000000000 +0600 +++ ./lib/sqlalchemy/engine/base.py 2010-10-21 12:53:13.000000000 +0600 @@ -21,6 +21,10 @@ 'connection_memoize']

import inspect, StringIO, sys, operator +try: + import collections +except ImportError: + collections = false from itertools import izip from sqlalchemy import exc, schema, util, types, log from sqlalchemy.sql import expression @@ -2003,6 +2007,11 @@ def itervalues(self): return iter(self)

+# Registering row proxy as Sequence type. +# Methods 'init', 'getitem', 'len' are provided, +# so sequence protocol is implemented +if collections: + collections.Sequence.register(RowProxy)

class ResultMetaData(object): """Handle cursor.description, applying additional info from an execution }}

[Former user]

If we can't wake up ged on this ticket If so, let's make some loud noises to wake him up.

Docs say about type checking here. To pass new style type checking (someone will definitely check that way some day) we should register our class as sequence type (thanks to dash at #python on freenode).

diff -ru /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c ./lib/sqlalchemy/cextension/resultproxy.c
--- /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/cextension/resultproxy.c   2010-07-15 22:35:09.000000000 +0600
+++ ./lib/sqlalchemy/cextension/resultproxy.c   2010-10-21 11:28:45.000000000 +0600
@@ -327,6 +327,12 @@
 }

 static PyObject *
+BaseRowProxy_getitem(PyObject *self, Py_ssize_t i)
+{
+    return BaseRowProxy_subscript((BaseRowProxy*)self, PyInt_FromSsize_t(i));
+}
+
+static PyObject *
 BaseRowProxy_getattro(BaseRowProxy *self, PyObject *name)
 {
     PyObject *tmp;
@@ -506,7 +512,7 @@
     (lenfunc)BaseRowProxy_length,   /* sq_length */
     0,                              /* sq_concat */
     0,                              /* sq_repeat */
-    0,                              /* sq_item */
+    (ssizeargfunc)BaseRowProxy_getitem,           /* sq_item */
     0,                              /* sq_slice */
     0,                              /* sq_ass_item */
     0,                              /* sq_ass_slice */
diff -ru /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/engine/base.py ./lib/sqlalchemy/engine/base.py
--- /home/nvetoshkin/SQLAlchemy-0.6.3/lib/sqlalchemy/engine/base.py     2010-07-15 22:35:10.000000000 +0600
+++ ./lib/sqlalchemy/engine/base.py     2010-10-21 12:53:13.000000000 +0600
@@ -21,6 +21,10 @@
     'connection_memoize']

 import inspect, StringIO, sys, operator
+try:
+    import collections
+except ImportError:
+    collections = false
 from itertools import izip
 from sqlalchemy import exc, schema, util, types, log
 from sqlalchemy.sql import expression
@@ -2003,6 +2007,11 @@
     def itervalues(self):
         return iter(self)

+# Registering row proxy as Sequence type.
+# Methods '__init__', '__getitem__', '__len__' are provided,
+# so sequence protocol is implemented
+if collections:
+    collections.Sequence.register(RowProxy)

 class ResultMetaData(object):
     """Handle cursor.description, applying additional info from an execution

P.S. Sorry for my previous unformatted answer.

nikita.vetoshkin

[Former user]

(original author: ged) Oups. Didn't see your message from 5 weeks ago. That new patch seems to go in the good direction. I'll test it as soon as I can find the time and I'll try to produce some tests too.

[Former user]

(original author: ged) Tests are positive. As far as I am concerned, the latest version of the patch can be applied as is. Here is a test case which test that it passes both the Python check and C level check (this is just the example Nikita gave us stripped down to what is really needed). I don't have time to make this into a proper test integrated into SA test suite. Mike, could you take over from here? Thank you Nikita for your work and patience.

[Former user]

I'm glad it was useful! :)

nikita.vetoshkin

[Former user]

We've already missed 0.6.5 milestone. zzzeek, could you please change Progress State from "needs questions answered" to something more suitable?

[Mike Bayer]

ged's test needs to be turned into a patch against the test suite.

[Former user]

patch + test case from ged

[Former user]

Made an attempt to add test case to the patch.

nikita.vetoshkin

[Mike Bayer]

thanks for the patch, a41c50ad63f688cce99fdb9920c4f7c24ef0c866

[Mike Bayer]

...and , fixes so the imports don't fail on 2.4, 2.5 e1a30715d233bc0aa2741502ab122cabf586b4c4 (see this is the time consuming stuff I need people to do for me)

[Former user]

I'll do my best next time to check other python versions

nikita.vetoshkin

[Mike Bayer]

well I'm already set up with python versions 2.4-> 3.2 here so wasnt a big deal, plus the buildbot helps

[Mike Bayer]

Removing milestone: 0.6.6 (automated comment)