Merge lp:~therve/storm/update-multi-tables into lp:storm
Proposed by
Thomas Herve
Status: | Work in progress |
---|---|
Proposed branch: | lp:~therve/storm/update-multi-tables |
Merge into: | lp:storm |
Diff against target: |
187 lines (+92/-15) 5 files modified
storm/databases/mysql.py (+9/-3) storm/databases/postgres.py (+11/-4) storm/expr.py (+43/-7) storm/store.py (+1/-1) tests/store/base.py (+28/-0) |
To merge this branch: | bzr merge lp:~therve/storm/update-multi-tables |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gustavo Niemeyer | Needs Fixing | ||
Storm Developers | Pending | ||
Review via email: mp+40790@code.launchpad.net |
Description of the change
I'm a bit fuzzy about the state.context values. The work done on the alive cache is not awesome as well.
To post a comment you must log in.
Unmerged revisions
- 385. By Thomas Herve
-
Reduce duplication
- 384. By Thomas Herve
-
Attemp implementing multi table update for postgres and mysql
Interesting changes, thanks Thomas. Here we go with some comments:
[1]
36 + if update.where is not Undef: update. where, state, raw=True)
37 + state.context = EXPR
38 + where = compile(
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.