Code review comment for lp:~allenap/storm/get-set-expr-columns

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

In most cases, I've used the replace_columns() helper in the store code where I need to fiddle with the column list for a query.

It'd probably be better to do the same in ResultSet.values(). The current code looks like it could leak changes outside of the method call, which could be problematic.

Secondly, I don't believe this patch will result in correct behaviour from ResultSet.values() for set expressions. Consider a table Foo containing the following data:

 id | name
 ---+------
 10 | value
 20 | value

And then do the following:

 result = store.find(Foo, id=10).union(store.find(Foo, id=20))
 print list(result.values(Foo.name))

This will print ['value'] when it should print ['value', 'value']. The result set clearly has two rows, but if we rewrite the column lists to Foo.name then the union will only give us one row.

review: Needs Fixing

« Back to merge proposal