Storm

Merge lp://staging/~cr3/storm/reference_is_in into lp://staging/storm

  1. reference_is_in
  2. Merge into trunk
Proposed by Marc Tardif
Status: Work in progress
Proposed branch: lp://staging/~cr3/storm/reference_is_in
Merge into: lp://staging/storm
Diff against target: 50 lines
To merge this branch: bzr merge lp://staging/~cr3/storm/reference_is_in
Reviewer Review Type Date Requested Status
James Henstridge Needs Fixing
Storm Developers Pending
Review via email: mp+5120@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Marc Tardif (cr3) wrote :

Proposing is_method to Reference class so that it would be possible to call this:

  Computer.account.is_in(account_objects)

Instead of

  Computer.account_id.is_in([a.id for a in account_objects])

Reply
Revision history for this message
James Henstridge (jamesh) wrote :

The implementation looks wrong for compound key references when a list of objects is passed in:

    for i, other in enumerate(others):
        where = self._relation.get_where_for_local(other)
        others[i] = where.expr2

In such a case, get_where_for_local() will return an And() object, which has no expr2 attribute.

Probably the best you can do in such a situation would be something like:

    return Or(*[self._relation.get_where_for_local(other)
                for other in others])

You can do better for PostgreSQL and MySQL with "(key1, key2) in ((val1, val2), (val1', val2'), ...)" but that doesn't seem to work with SQLite.

You'd also want to make sure it does the right thing when passed an empty list.

review: Needs Fixing
Reply
Revision history for this message
Marc Tardif (cr3) wrote :

Interestingly, I had originally implemented the is_in method on references using the Or expression but I later found it preferable to use the In expression. However, in order to support a wider range of backends, I'm fine with reverting to the Or expression.

Thanks for suggesting to test the corner case when passing an empty list. I made sure to write the corresponding test which then failed when just uring the Or expression. So, when the given argument to the is_in method is an empty list, it simply returns False which seems to work fine.

The branch has been updated with these changes.

Reply
lp://staging/~cr3/storm/reference_is_in updated
306. By Marc Tardif

Fixed is_in method in references to support compound key references and added test when passed an empty list.

Revision history for this message
James Henstridge (jamesh) wrote :

> Interestingly, I had originally implemented the is_in method on references
> using the Or expression but I later found it preferable to use the In
> expression. However, in order to support a wider range of backends, I'm fine
> with reverting to the Or expression.
>
> Thanks for suggesting to test the corner case when passing an empty list. I
> made sure to write the corresponding test which then failed when just uring
> the Or expression. So, when the given argument to the is_in method is an empty
> list, it simply returns False which seems to work fine.
>
> The branch has been updated with these changes.

With your new changes, passing an Expr object to is_in() will now fail. With the previous version of your patch it was possible to do something like:

    Employee.company.is_in(Select(Company.id, Company.name.like("%corp%")))

The cast to list() in your new version would fail here.

Reply
Revision history for this message
Jamu Kakar (jkakar) wrote :

Is anything going on with this branch?

Reply

Unmerged revisions

306. By Marc Tardif

Fixed is_in method in references to support compound key references and added test when passed an empty list.

305. By Marc Tardif

Added is_in method to Reference class.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/references.py'
2--- storm/references.py 2009-02-19 16:44:33 +0000
3+++ storm/references.py 2009-08-13 20:20:47 +0000
4@@ -22,7 +22,7 @@
5 from storm.store import Store, get_where_for_args
6 from storm.variables import LazyValue
7 from storm.expr import (
8- Select, Column, Exists, ComparableExpr, LeftJoin, Not, SQLRaw,
9+ Select, Column, Exists, ComparableExpr, LeftJoin, Or, Not, SQLRaw,
10 compare_columns, compile)
11 from storm.info import get_cls_info, get_obj_info
12
13@@ -203,6 +203,14 @@
14 def __ne__(self, other):
15 return Not(self == other)
16
17+ def is_in(self, others):
18+ others = list(others)
19+ if not others:
20+ return False
21+
22+ return Or(*[self._relation.get_where_for_local(other)
23+ for other in others])
24+
25
26 class ReferenceSet(object):
27
28
29=== modified file 'tests/store/base.py'
30--- tests/store/base.py 2009-07-31 01:53:08 +0000
31+++ tests/store/base.py 2009-08-13 20:20:47 +0000
32@@ -3008,6 +3008,18 @@
33 LinkWithRef, LinkWithRef.myself != (link.foo_id, link.bar_id)))
34 self.assertTrue(link not in result, "%r not in %r" % (link, result))
35
36+ def test_reference_is_in(self):
37+ foo1 = self.store.get(Foo, 10)
38+ foo2 = self.store.get(Foo, 20)
39+
40+ result = self.store.find(Bar, Bar.foo.is_in([foo1, foo2]))
41+ self.assertEquals(result.count(), 2)
42+ self.assertEquals([100, 200], sorted(bar.id for bar in result))
43+
44+ def test_reference_is_in_with_empty(self):
45+ result = self.store.find(Bar, Bar.foo.is_in([]))
46+ self.assertEquals(result.count(), 0)
47+
48 def test_reference_self(self):
49 selfref = self.store.add(SelfRef())
50 selfref.id = 400

Subscribers

People subscribed via source and target branches

to status/vote changes: