Merge lp:~therve/storm/update-multi-tables into lp:storm

[Thomas Herve]

I'm a bit fuzzy about the state.context values. The work done on the alive cache is not awesome as well.

[Gustavo Niemeyer (niemeyer)]

Interesting changes, thanks Thomas. Here we go with some comments:

[1]

36 + if update.where is not Undef:
37 + state.context = EXPR
38 + where = compile(update.where, state, raw=True)
39 + if state.auto_tables:
40 + tables = set(

You were right to wonder about the context. Note that above the table set will potentially be compiled as EXPR expression, rather than TABLE. Same thing in the other version of compile.

[2]

43 + # Override the table token
44 + tokens[1] = ", ".join(tables)

This feels slightly hackish. We just went through the trouble of compiling the
tokens[1] value above. If we don't need it, the code should be organized in
such a way that tables are compiled just once, and in the same place where
the decision to compile it one way or another is taken.

Also, note that build_tables() internally is able to verify that the auto_tables
is available or not, and build it rather than a default set of tables conditionally,
so it feels like this work doesn't even have to be performed at this level, and may
instead be fully handed into build_tables().

[3]

96 + tables.remove(tokens[1])
97 + if tables:
98 + tokens.append(" FROM ")
99 + tokens.append(", ".join(tables))

Same case as above, but this time build_tables() won't be able to work alone.

[4]

It feels that the distinction between the two compilations of Update is quite small
considering the overall size. Can't we have the default (in expr.py) version which
compiles Update expressions accepting a boolean parameter to act one way or the
other, so that we can keep a single implementation and just select a flag in the
subcompiler?

[5]

118 - except CompileError:
119 + except (CompileError, KeyError):

Your gut feeling was also right on this one. This new KeyError here is quite
mysterious. I don't even understand myself where it's coming from, to be honest,
so we should find a better way to avoid this from popping up.

review: Needs Fixing

[lp:~therve/storm/update-multi-tables]

385. By Thomas Herve on 2010-12-11

Reduce duplication

[James Henstridge (jamesh)]

I made a similar comment on the delete-with-using branch, but I think it'd be good to build the "FROM" clause using build_tables rather than accessing auto_tables manually. This would probably involve adding a way to tell it to exclude the primary table for the update from its output.

Unmerged revisions

385. By Thomas Herve on 2010-12-11

Reduce duplication

384. By Thomas Herve on 2010-11-13

Attemp implementing multi table update for postgres and mysql

=== modified file 'storm/databases/mysql.py'
--- storm/databases/mysql.py	2009-09-21 17:03:08 +0000
+++ storm/databases/mysql.py	2010-12-11 13:15:10 +0000
@@ -31,13 +31,12 @@
     MySQLdb = dummy
 
 from storm.expr import (
-    compile, Insert, Select, compile_select, Undef, And, Eq,
-    SQLRaw, SQLToken, is_safe_token)
+    compile, Insert, Select, Update, compile_select, Undef, SQLToken,
+    is_safe_token, compile_update, _MULTIPLE_UPDATE_INLINE)
 from storm.variables import Variable
 from storm.database import Database, Connection, Result
 from storm.exceptions import (
     install_exceptions, DatabaseModuleError, OperationalError)
-from storm.variables import IntVariable
 
 
 install_exceptions(MySQLdb)
@@ -51,6 +50,13 @@
         select.limit = sys.maxint
     return compile_select(compile, select, state)
 
+
[e-mail](Update)
+def compile_mysql_update(compile, update, state):
+    return compile_update(compile, update, state,
+                          multi_tables=_MULTIPLE_UPDATE_INLINE)
+
+
 @compile.when(SQLToken)
 def compile_sql_token_mysql(compile, expr, state):
     """MySQL uses ` as the escape character by default."""

=== modified file 'storm/databases/postgres.py'
--- storm/databases/postgres.py	2010-10-23 21:02:22 +0000
+++ storm/databases/postgres.py	2010-12-11 13:15:10 +0000
@@ -39,10 +39,11 @@
     psycopg2 = dummy
 
 from storm.expr import (
-    Undef, Expr, SetExpr, Select, Insert, Alias, And, Eq, FuncExpr, SQLRaw,
-    Sequence, Like, SQLToken, COLUMN, COLUMN_NAME, COLUMN_PREFIX, TABLE,
-    compile, compile_select, compile_insert, compile_set_expr, compile_like,
-    compile_sql_token)
+    Undef, Expr, SetExpr, Select, Insert, Update, Alias, And, Eq, FuncExpr,
+    SQLRaw, Sequence, Like, SQLToken, COLUMN, COLUMN_NAME, COLUMN_PREFIX,
+    TABLE, compile, compile_select, compile_insert, compile_set_expr,
+    compile_like, compile_sql_token, compile_update,
+    _MULTIPLE_UPDATE_WITH_FROM)
 from storm.variables import Variable, ListVariable
 from storm.database import Database, Connection, Result
 from storm.exceptions import (
@@ -183,6 +184,12 @@
     return compile_insert(compile, insert, state)
 
 
[e-mail](Update)
+def compile_postgres_update(compile, update, state):
+    return compile_update(compile, update, state,
+                          multi_tables=_MULTIPLE_UPDATE_WITH_FROM)
+
+
 @compile.when(Sequence)
 def compile_sequence_postgres(compile, sequence, state):
     return "nextval('%s')" % sequence.name

=== modified file 'storm/expr.py'
--- storm/expr.py	2010-12-07 15:00:30 +0000
+++ storm/expr.py	2010-12-11 13:15:10 +0000
@@ -741,21 +741,57 @@
         self.table = table
         self.default_table = default_table
 
+
+_MULTIPLE_UPDATE_UNSUPPORTED = 0
+_MULTIPLE_UPDATE_WITH_FROM = 1
+_MULTIPLE_UPDATE_INLINE = 2
+
+
 @compile.when(Update)
-def compile_update(compile, update, state):
+def compile_update(compile, update, state,
+                   multi_tables=_MULTIPLE_UPDATE_UNSUPPORTED):
+    """
+    @param multi_tables: if multi-tables update is supported by the DB engine
+        as a SQL extension, pass _MULTIPLE_UPDATE_WITH_FROM to use the syntax
+        with FROM, or _MULTIPLE_UPDATE_INLINE to use the inline syntax.
+    """
     map = update.map
-    state.push("context", COLUMN_NAME)
+    original_context = COLUMN_NAME
+    if multi_tables == _MULTIPLE_UPDATE_INLINE:
+        original_context = TABLE
+    state.push("context", original_context)
     sets = ["%s=%s" % (compile(col, state, token=True),
                        compile(map[col], state))
             for col in map]
-    state.context = TABLE
-    tokens = ["UPDATE ", build_tables(compile, update.table,
-                                      update.default_table, state),
-              " SET ", ", ".join(sets)]
+    where = None
     if update.where is not Undef:
         state.context = EXPR
+        where = compile(update.where, state, raw=True)
+    state.context = TABLE
+    if multi_tables and state.auto_tables:
+        if multi_tables == _MULTIPLE_UPDATE_WITH_FROM:
+            first_table = build_tables(
+                compile, update.table, update.default_table, state)
+            other_tables = set(
+                compile(table, state, token=True)
+                for table in state.auto_tables)
+            tokens = ["UPDATE ", first_table, " SET ", ", ".join(sets)]
+            # Omit the original target
+            other_tables.remove(first_table)
+            if other_tables:
+                tokens.append(" FROM ")
+                tokens.append(", ".join(other_tables))
+        elif multi_tables == _MULTIPLE_UPDATE_INLINE:
+            tables = build_tables(
+                compile, Undef, update.default_table, state)
+            tokens = ["UPDATE ", tables, " SET ", ", ".join(sets)]
+    else:
+        tables = build_tables(
+            compile, update.table, update.default_table, state)
+        tokens = ["UPDATE ", tables, " SET ", ", ".join(sets)]
+    if where is not None:
         tokens.append(" WHERE ")
-        tokens.append(compile(update.where, state, raw=True))
+        tokens.append(where)
     state.pop()
     return "".join(tokens)
 

=== modified file 'storm/store.py'
--- storm/store.py	2010-10-14 20:54:44 +0000
+++ storm/store.py	2010-12-11 13:15:10 +0000
@@ -1375,7 +1375,7 @@
 
         try:
             cached = self.cached()
-        except CompileError:
+        except (CompileError, KeyError):
             # We are iterating through all objects in memory here, so
             # check if the object type matches to avoid trying to
             # invalidate a column that does not exist, on an unrelated

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2010-10-19 13:36:26 +0000
+++ tests/store/base.py	2010-12-11 13:15:10 +0000
@@ -2724,6 +2724,34 @@
         self.assertFalse(hasattr(foo, "tainted"))
         self.assertEquals(foo.title, "Title 40")
 
+    def test_find_set_with_two_tables(self):
+        """
+        Postgres and MySQL supports updating a table using other tables as
+        filter.
+        """
+        cls_name = self.__class__.__name__
+        if cls_name.startswith("SQLite"):
+            return
+        result = self.store.find(
+            Foo, Foo.id == Bar.foo_id, Bar.title == u"Title 200")
+        result.set(title=u"Title 2a")
+        self.assertEqual(u"Title 2a", self.store.get(Foo, 20).title)
+
+    def test_find_set_with_multiple_tables(self):
+        """
+        Update works with multiple tables in Postgres and MySQL.
+        """
+        cls_name = self.__class__.__name__
+        if cls_name.startswith("SQLite"):
+            return
+        result = self.store.find(
+            Foo, Foo.id == Link.foo_id, Link.bar_id == Bar.id,
+            Bar.title == u"Title 200")
+        # Access the result set here, to cache the objects
+        data = list(result)
+        result.set(title=u"Title 2a")
+        self.assertEqual(u"Title 2a", self.store.get(Foo, 20).title)
+
     def test_reference(self):
         bar = self.store.get(Bar, 100)
         self.assertTrue(bar.foo)