Skip to content

Commit

Permalink
Revert to using OrderedDict.
Browse files Browse the repository at this point in the history
  • Loading branch information
pudo committed Feb 18, 2014
1 parent a963e4a commit 6ef4cd7
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 7 deletions.
6 changes: 2 additions & 4 deletions dataset/persistence/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from sqlalchemy.schema import Column, Index
from sqlalchemy import alias
from dataset.persistence.util import guess_type
from dataset.persistence.util import ResultIter
from dataset.persistence.util import ResultIter, convert_row
from dataset.util import DatasetException


Expand Down Expand Up @@ -282,9 +282,7 @@ def find_one(self, **_filter):
args = self._args_to_clause(_filter)
query = self.table.select(whereclause=args, limit=1)
rp = self.database.executable.execute(query)
data = rp.fetchone()
if data is not None:
return data
return convert_row(rp.fetchone())

def _args_to_order_by(self, order_by):
if order_by[0] == '-':
Expand Down
15 changes: 14 additions & 1 deletion dataset/persistence/util.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
from datetime import datetime, timedelta
from inspect import isgenerator

try:
from collections import OrderedDict
except ImportError:
from ordereddict import OrderedDict

from sqlalchemy import Integer, UnicodeText, Float, DateTime, Boolean, types, Table, event


Expand All @@ -16,6 +21,12 @@ def guess_type(sample):
return UnicodeText


def convert_row(row):
if row is None:
return None
return OrderedDict(row.items())


class ResultIter(object):
""" SQLAlchemy ResultProxies are not iterable to get a
list of dictionaries. This is to wrap them. """
Expand Down Expand Up @@ -47,7 +58,7 @@ def __next__(self):
else:
# stop here
raise StopIteration
return row
return convert_row(row)

next = __next__

Expand All @@ -66,6 +77,8 @@ def process_bind_param(self, value, dialect):
return (value / 1000 - self.epoch).total_seconds()

def process_result_value(self, value, dialect):
if isinstance(value, basestring):
return value
return self.epoch + timedelta(seconds=value / 1000)

def is_sqlite(inspector):
Expand Down
6 changes: 4 additions & 2 deletions test/test_persistence.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,11 @@ def test_query(self):

def test_table_cache_updates(self):
tbl1 = self.db.get_table('people')
tbl1.insert(OrderedDict([('first_name', 'John'), ('last_name', 'Smith')]))
data = OrderedDict([('first_name', 'John'), ('last_name', 'Smith')])
tbl1.insert(data)
data['id'] = 1
tbl2 = self.db.get_table('people')
assert list(tbl2.all()) == [(1, 'John', 'Smith')]
assert dict(tbl2.all().next()) == dict(data), (tbl2.all().next(), data)


class TableTestCase(unittest.TestCase):
Expand Down

4 comments on commit 6ef4cd7

@hszcg
Copy link
Contributor

@hszcg hszcg commented on 6ef4cd7 Aug 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @pudo , just curious that why we add back OrderedDict here?

I saw the discussion on #47 and patch commit on 0726dd9 , but not very clear why we added this back, given the fact that OrderedDict is much slower than dict?

Thanks,

@pudo
Copy link
Owner Author

@pudo pudo commented on 6ef4cd7 Aug 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I really can't remember, but I don't think that I am using the fact it's an ordered dict anywhere. Do you have a link re performance?

@hszcg
Copy link
Contributor

@hszcg hszcg commented on 6ef4cd7 Aug 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gka
Copy link
Collaborator

@gka gka commented on 6ef4cd7 Aug 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do the performance testing with OrderedDict vs. RowProxy instead of comparing with dicts, which don't expose the keys in order.

That being said, since RowProxy objects are having the decency of re-using a single list for storing (obviously identical) ordered keys for thousands of rows in a result I think it doesn't make much sense to cast to OrderedDicts.

Please sign in to comment.