Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves SQLAlchemy has_key method using SQL EXISTS clause #18

Merged
merged 1 commit into from
Oct 9, 2014

Conversation

jvrsantacruz
Copy link
Contributor

It uses SQL EXISTS clause instead of fetching a record and comparing it against None.

See the current query:

 return None != self.bind.execute(
        select([self.table.c.key], self.table.c.key == key).limit(1)
    ).first()

This fragment of code can result a bit problematic due to de comparision
of a sqlalchemy RowProxy against None, which should be using the is operator also.

The proposed query:

return self.bind.execute(
    select([exists().where(self.table.c.key == key)])
).scalar()

Returns a boolean directly from the database,
and it should be allegedly faster while keeping it ANSI SQL.

Congrats for the library, it's easy to use and fun to extend.

It uses SQL EXISTS clause instead of fetching a record and comparing it against None.

The current function:

     return None != self.bind.execute(
            select([self.table.c.key], self.table.c.key == key).limit(1)
        ).first()

This fragment of code can result a bit problematic due to de comparision
of a sqlalchemy RowProxy against None, which should be using the `is` operator also.

The proposed query:

    return self.bind.execute(
        select([exists().where(self.table.c.key == key)])
    ).scalar()

Returns a boolean directly from the database,
and it should be allegedly faster while keeping it ANSI SQL.
@mbr
Copy link
Owner

mbr commented Oct 9, 2014

Good catch. I'll merge this - have you run into any issues with this or can this wait until the next release?

mbr added a commit that referenced this pull request Oct 9, 2014
Improves SQLAlchemy has_key method using SQL EXISTS clause.
@mbr mbr merged commit 704b89f into mbr:master Oct 9, 2014
@jvrsantacruz
Copy link
Contributor Author

Well, now that you mention it, I looked into this because of this exception:

ipdb> None != self.bind.execute(
   select([self.table.c.key], self.table.c.key == key).limit(1)).first()
*** TypeError: 'NoneType' object is not iterable

Where the value returned by execute is a sqlalchemy RowProxy:

ipdb> value = self.bind.execute(
  select([self.table.c.key], self.table.c.key == key).limit(1)).first()
ipdb> value
(u'httphost',)
ipdb> type(value)
<class 'sqlalchemy.engine.result.RowProxy'>
ipdb> None != value
*** TypeError: 'NoneType' object is not iterable

This happens using SQLAlchemy (0.9.0).
After I upgraded to SQLAlchemy (0.9.7), problem was solved, and it behaves as expected.

So no, there is no hurry to get this out in a new version, I can work perfectly well.
Were someone to run into this very same error, the only thing it has to do is upgrade versions.

Anyways, the None against RowProxy comparision didn't felt like a very trustable behaviour to me, so I thougth that getting a good old bool was safer (and a little bit faster).

Thanks!

beetleman pushed a commit to beetleman/simplekv that referenced this pull request Nov 1, 2014
Improves SQLAlchemy has_key method using SQL EXISTS clause.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants