Merge lp:~jkakar/storm/resultset-expression into lp:storm

[Jamu Kakar]

[Jamu Kakar (jkakar)]

This branch makes ResultSet a subclass of Expr. It also adds a
compiler specialization to generate SQL from a ResultSet. This
makes it easy to use a ResultSet as a subselect. Unfortunately, the
changes are not 100% backwards compatible. Code such as this:

  result1 = store.find(Foo)
  result2 = store.find(Foo, Foo.id.is_in(result1))

will now result in an OperationalError when run against all
currently supported databases. This is because, before the changes
made in this branch, result1 would have been iterated and yielded
Foo instances. The changes in this branch result in an invalid
query being generated for this case, because the subselect returns
all foo columns, instead of just the id column.

The current behaviour seems a bit odd, so I'm inclined to not worry
about the issue.

[Guilherme Salgado (salgado)]

At first I found it odd to think of a ResultSet as an Expr, but since it's actually just a container for a Select (which is an Expr), I guess it makes sense.

I think it'd be good to document the behaviour change in the NEWS file, what do you think?

review: Approve

[James Henstridge (jamesh)]

I'm not sure if this branch actually helps with the relevant bug.

It doesn't look like it'd help with the example you've given above, which would generate SQL something like:

  SELECT Foo.id, Foo.column1, Foo.column2, ...
    FROM Foo
    WHERE Foo.id IN
      (SELECT Foo.id, Foo.column1, Foo.column2, ...
         FROM Foo);

This is obviously not what was intended. We can fix this by changing the first find call to "result1 = store.find(Foo.id)", but if we're in a position to do that, then we could just as easily have created a Select object directly.

My understanding is that the intended use is in cases where you are using an API that just returns a ResultSet, and these APIs rarely let you specify what is being selected.

Out of all the possible solutions to the problem, I liked the original ResultSet.select(columns) proposal most.

review: Needs Fixing

[Jamu Kakar (jkakar)]

James:

Thanks for looking at the changes! I think you might have
misunderstood my original comment: I was saying the same thing you
are, that code that used to work no longer will. Also, I do think
this branch helps with the original problem, but agree that it can
be tricky to use.

That said, it's very easy to generate bad queries and, also, very
easy to inadvertently use result set from one store with a find on a
different store. I think the problems with this approach outweigh
the benefits. The more I think about it the more I like the
ResultSet.select approach, or even just making _get_select a public
method. They are at least very simple and don't change the current
semantics in any way.

[James Henstridge (jamesh)]

Okay. I see what you meant now. Note that the example you gave doesn't actually work with Storm trunk either, so I don't think there are backward compatibility issues to worry about:

  >>> result1 = store.find(Foo)
  >>> result2 = store.find(Foo, Foo.id.is_in(result1))
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "storm/expr.py", line 496, in is_in
      others[i] = variable_factory(value=other)
    File "storm/variables.py", line 342, in parse_set
      % (type(value), value))
  TypeError: Expected int, found <class '__main__.Foo'>: <__main__.Foo object at 0x2a462d0>

The other problem I have with this branch is that if I pass a table to store.find(), the columns in the select will not be in a meaningful order. While it is fine for the store code to use class dictionary iteration order internally, I don't think we want to expose this ordering to users.

Unmerged revisions

355. By Jamu Kakar on 2009-12-30

- Change a test using a ResultSet with is_in() to request a specific
  column.

354. By Jamu Kakar on 2009-12-30

- ResultSet is now a subclass of Expr and the compiler is
  specialized to generate SQL for it.

=== modified file 'storm/store.py'
--- storm/store.py	2009-11-29 00:42:19 +0000
+++ storm/store.py	2009-12-30 10:17:14 +0000
@@ -33,7 +33,7 @@
 from storm.expr import (
     Expr, Select, Insert, Update, Delete, Column, Count, Max, Min,
     Avg, Sum, Eq, And, Asc, Desc, compile_python, compare_columns, SQLRaw,
-    Union, Except, Intersect, Alias, SetExpr)
+    Union, Except, Intersect, Alias, SetExpr, compile)
 from storm.exceptions import (
     WrongStoreError, NotFlushedError, OrderLoopError, UnorderedError,
     NotOneError, FeatureError, CompileError, LostObjectError, ClassInfoError)
@@ -892,7 +892,7 @@
                 obj_info.variables[column].checkpoint()
 
 
-class ResultSet(object):
+class ResultSet(Expr):
     """The representation of the results of a query.
 
     Note that having an instance of this class does not indicate that
@@ -1437,6 +1437,15 @@
         return self._set_expr(Intersect, other, all)
 
 
[e-mail](ResultSet)
+def compile_result_set(compile, result, state):
+    """Generate SQL for a L{ResultSet}."""
+    # Reset state precendence to 0, as it would be if the compiler didn't
+    # think this was a nested expression.
+    state.precedence = 0
+    return compile(result._get_select(), state)
+
+
 class EmptyResultSet(object):
     """An object that looks like a L{ResultSet} but represents no rows.
 

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2009-11-30 11:35:02 +0000
+++ tests/store/base.py	2009-12-30 10:17:14 +0000
@@ -25,12 +25,13 @@
 import weakref
 
 from storm.references import Reference, ReferenceSet, Proxy
-from storm.database import Result
+from storm.database import Result, create_database
 from storm.properties import Int, Float, RawStr, Unicode, Property, Pickle
 from storm.properties import PropertyPublisherMeta, Decimal
 from storm.variables import PickleVariable
 from storm.expr import (
-    Asc, Desc, Select, LeftJoin, SQL, Count, Sum, Avg, And, Or, Eq, Lower)
+    Asc, Desc, Select, LeftJoin, SQL, Count, Sum, Avg, And, Or, Eq, Lower,
+    compile)
 from storm.variables import Variable, UnicodeVariable, IntVariable
 from storm.info import get_obj_info, ClassAlias
 from storm.exceptions import *
@@ -5340,7 +5341,7 @@
         self.assertEquals(result3.count(), 2)
 
     def test_is_in_empty_result_set(self):
-        result1 = self.store.find(Foo, Foo.id < 10)
+        result1 = self.store.find(Foo.id, Foo.id < 10)
         result2 = self.store.find(Foo, Or(Foo.id > 20, Foo.id.is_in(result1)))
         self.assertEquals(result2.count(), 1)
 
@@ -5709,6 +5710,37 @@
         self.assertEquals(result_to_remove.remove(), 3)
 
 
+class ResultSetTest(TestHelper):
+
+    def setUp(self):
+        super(ResultSetTest, self).setUp()
+        self.store = Store(create_database("sqlite:"))
+
+    def tearDown(self):
+        self.store.close()
+        super(ResultSetTest, self).tearDown()
+
+    def test_compile(self):
+        """
+        The compiler is specialized so that L{ResultSet}s can be used with the
+        L{expression system<storm.expr>}.
+        """
+        result = self.store.find(Foo)
+        self.assertEqual(u"SELECT foo.id, foo.title FROM foo",
+                         compile(result))
+
+    def test_compile_subselect(self):
+        """
+        When a L{ResultSet} is used as a subselect the compiler will generate
+        a nested SELECT statement.
+        """
+        result1 = self.store.find(Foo.id)
+        result2 = self.store.find(Foo, Foo.id.is_in(result1))
+        self.assertEqual(u"SELECT foo.id, foo.title FROM foo "
+                         u"WHERE foo.id IN (SELECT foo.id FROM foo)",
+                         compile(result2))
+
+
 class EmptyResultSetTest(object):
 
     def setUp(self):