Merge lp:~allenap/storm/value-columns-by-name into lp:storm
- value-columns-by-name
- Merge into trunk
Status: | Work in progress |
---|---|
Proposed branch: | lp:~allenap/storm/value-columns-by-name |
Merge into: | lp:storm |
Diff against target: |
182 lines (+107/-9) 2 files modified
storm/store.py (+40/-8) tests/store/base.py (+67/-1) |
To merge this branch: | bzr merge lp:~allenap/storm/value-columns-by-name |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gustavo Niemeyer | Disapprove | ||
Jamu Kakar (community) | Needs Fixing | ||
James Henstridge | Needs Fixing | ||
Review via email: mp+23480@code.launchpad.net |
Commit message
ResultSet.values() can now accept column names as well as columns themselves.
Description of the change
In an environment like Launchpad where a lot of the code only uses Storm via a Zope prophylactic, and there is also an importfascist that complains bitterly when model code is imported by non-model modules, it would be extremely handy to be able to pass names into ResultSet.values() rather than columns.
I might have done this all the wrong way, but it's a start and I'm happy to learn the right way to get this branch landed.
James Henstridge (jamesh) wrote : | # |
If we want to support this feature, it should probably exclude all tuple finds. For example:
result = store.find(
ids = result.values('id')
Assuming both tables have an 'id' column, which one am I referring to? It seems better to limit the feature to the simple situations where we can handle it unambiguously.
James Henstridge (jamesh) wrote : | # |
And a follow up: to be consistent with Store.find(), I'd suggest using getattr(
In the tuple find case, default_cls will be None, so you should check for this and raise FeatureError.
Jamu Kakar (jkakar) wrote : | # |
[1]
Can you please add docstrings to the tests. Also, the @param for
'columns' in the docstring for ResultSet.values needs to be updated.
[2]
+ def test_find_
There are several different cases being exercised in this test. I
recommend you break it up into several smaller tests.
[3]
+ # If more than one column in the result set has the same name,
+ # the first will be chosen.
+ result = self.store.find(
+ (Foo.id, FooValue.id), FooValue.foo_id == Foo.id)
+ result = result.
+ values = result.values('id')
+ self.assertEqua
I agree with James that this case should raise FeatureError. We
shouldn't be making guesses about which column the user is
specifying if the choice is ambiguous.
[4]
+ def test_find_
This test could also be split into two, so that each test exercises
one specific behaviour (multiple columns, and mixed string and Column
values).
Gavin Panella (allenap) wrote : | # |
Getting columns from names is now done by getting the columns from the
find set, using it to construct a dict of name -> [columns] and
mapping with that. When the result of the mapping is a list of length
2 or more then it raises FeatureError. In this way it will work
naturally with tuple finds, and allow values() to (not quite, there's
another issue) yield expression values.
I've also changed it to copy the select before modifying it. I don't
know how necessary this is, but the code before looked a bit suspect
to my untrained eye.
Also, when variable_factory is not found on the column, it defaults to
Variable. This means that it's possible to do things like
results.
Okay, having said all that, if this is too fancy, or liable to cause
issues, I'll go for the approach in get_where_
However, if this approach is okay, a couple of notes:
- Either or both of the new methods ResultSet.
and ResultSet.
that be a more natural home for them?
- Should _map_column_names() be use in order_by() and group_by()?
1 | === modified file 'storm/store.py' | |||
2 | --- storm/store.py 2010-04-15 13:31:08 +0000 | |||
3 | +++ storm/store.py 2010-04-22 09:32:10 +0000 | |||
4 | @@ -540,7 +540,6 @@ | |||
5 | 540 | else: | 540 | else: |
6 | 541 | 541 | ||
7 | 542 | cached_primary_vars = obj_info["primary_vars"] | 542 | cached_primary_vars = obj_info["primary_vars"] |
8 | 543 | primary_key_idx = cls_info.primary_key_idx | ||
9 | 544 | 543 | ||
10 | 545 | changes = self._get_changes_map(obj_info) | 544 | changes = self._get_changes_map(obj_info) |
11 | 546 | 545 | ||
12 | @@ -1238,34 +1237,63 @@ | |||
13 | 1238 | """Get the sum of all values in an expression.""" | 1237 | """Get the sum of all values in an expression.""" |
14 | 1239 | return self._aggregate(Sum, expr, expr) | 1238 | return self._aggregate(Sum, expr, expr) |
15 | 1240 | 1239 | ||
16 | 1240 | def _get_column_name_map(self): | ||
17 | 1241 | """Return a mapping of column name to lists of columns.""" | ||
18 | 1242 | columns, tables = self._find_spec.get_columns_and_tables() | ||
19 | 1243 | column_map = {} | ||
20 | 1244 | for column in columns: | ||
21 | 1245 | if isinstance(column, (Alias, Column)): | ||
22 | 1246 | if column.name in column_map: | ||
23 | 1247 | column_map[column.name].append(column) | ||
24 | 1248 | else: | ||
25 | 1249 | column_map[column.name] = [column] | ||
26 | 1250 | return column_map | ||
27 | 1251 | |||
28 | 1252 | def _map_column_names(self, columns): | ||
29 | 1253 | """Attempt to map column names to actual columns.""" | ||
30 | 1254 | column_map = self._get_column_name_map() | ||
31 | 1255 | for column in columns: | ||
32 | 1256 | if column in column_map: | ||
33 | 1257 | column_name, column_list = column, column_map[column] | ||
34 | 1258 | if len(column_list) != 1: | ||
35 | 1259 | raise FeatureError("Ambiguous column: %s" % column_name) | ||
36 | 1260 | [column] = column_list | ||
37 | 1261 | if isinstance(column, Alias): | ||
38 | 1262 | column = column.expr | ||
39 | 1263 | yield column | ||
40 | 1264 | |||
41 | 1241 | def values(self, *columns): | 1265 | def values(self, *columns): |
42 | 1242 | """Retrieve only the specified columns. | 1266 | """Retrieve only the specified columns. |
43 | 1243 | 1267 | ||
44 | 1244 | This does not load full objects from the database into Python. | 1268 | This does not load full objects from the database into Python. |
45 | 1245 | 1269 | ||
48 | 1246 | @param columns: One or more L{storm.expr.Column} objects whose | 1270 | @param columns: One or more L{storm.expr.Column} objects, or |
49 | 1247 | values will be fetched. | 1271 | column names, whose values will be fetched. If column |
50 | 1272 | names are given, each must refer unambiguously to a named | ||
51 | 1273 | column or alias in the result set. | ||
52 | 1248 | @return: An iterator of tuples of the values for each column | 1274 | @return: An iterator of tuples of the values for each column |
53 | 1249 | from each matching row in the database. | 1275 | from each matching row in the database. |
54 | 1250 | """ | 1276 | """ |
55 | 1251 | if not columns: | 1277 | if not columns: |
56 | 1252 | raise FeatureError("values() takes at least one column " | 1278 | raise FeatureError("values() takes at least one column " |
57 | 1253 | "as argument") | 1279 | "as argument") |
64 | 1254 | select = self._get_select() | 1280 | columns = list(self._map_column_names(columns)) |
65 | 1255 | column_map = dict( | 1281 | # replace_columns() can lose ordering so it's not good here. |
66 | 1256 | (column.name, column) | 1282 | select = copy(self._get_select()) |
61 | 1257 | for column in reversed(select.columns) | ||
62 | 1258 | if isinstance(column, Column)) | ||
63 | 1259 | columns = [column_map.get(column, column) for column in columns] | ||
67 | 1260 | select.columns = columns | 1283 | select.columns = columns |
68 | 1261 | result = self._store._connection.execute(select) | 1284 | result = self._store._connection.execute(select) |
71 | 1262 | if len(columns) == 1: | 1285 | variable_factories = [ |
72 | 1263 | variable = columns[0].variable_factory() | 1286 | getattr(column, 'variable_factory', Variable) |
73 | 1287 | for column in columns] | ||
74 | 1288 | variables = [ | ||
75 | 1289 | variable_factory() | ||
76 | 1290 | for variable_factory in variable_factories] | ||
77 | 1291 | if len(variables) == 1: | ||
78 | 1292 | [variable] = variables | ||
79 | 1264 | for values in result: | 1293 | for values in result: |
80 | 1265 | result.set_variable(variable, values[0]) | 1294 | result.set_variable(variable, values[0]) |
81 | 1266 | yield variable.get() | 1295 | yield variable.get() |
82 | 1267 | else: | 1296 | else: |
83 | 1268 | variables = [column.variable_factory() for column in columns] | ||
84 | 1269 | for values in result: | 1297 | for values in result: |
85 | 1270 | for variable, value in zip(variables, values): | 1298 | for variable, value in zip(variables, values): |
86 | 1271 | result.set_variable(variable, value) | 1299 | result.set_variable(variable, value) |
87 | @@ -1370,7 +1398,6 @@ | |||
88 | 1370 | def get_column(column): | 1398 | def get_column(column): |
89 | 1371 | return obj_info.variables[column].get() | 1399 | return obj_info.variables[column].get() |
90 | 1372 | objects = [] | 1400 | objects = [] |
91 | 1373 | cls = self._find_spec.default_cls_info.cls | ||
92 | 1374 | for obj_info in self._store._iter_alive(): | 1401 | for obj_info in self._store._iter_alive(): |
93 | 1375 | try: | 1402 | try: |
94 | 1376 | if (obj_info.cls_info is self._find_spec.default_cls_info and | 1403 | if (obj_info.cls_info is self._find_spec.default_cls_info and |
95 | 1377 | 1404 | ||
96 | === modified file 'tests/store/base.py' | |||
97 | --- tests/store/base.py 2010-04-15 13:31:08 +0000 | |||
98 | +++ tests/store/base.py 2010-04-22 10:30:27 +0000 | |||
99 | @@ -1002,25 +1002,51 @@ | |||
100 | 1002 | self.assertEquals([type(value) for value in values], | 1002 | self.assertEquals([type(value) for value in values], |
101 | 1003 | [unicode, unicode, unicode]) | 1003 | [unicode, unicode, unicode]) |
102 | 1004 | 1004 | ||
103 | 1005 | def test_find_values_with_expression(self): | ||
104 | 1006 | """ | ||
105 | 1007 | An expression can be passed to ResultSet.values(). | ||
106 | 1008 | """ | ||
107 | 1009 | values = self.store.find(Foo.id).values(Sum(Foo.id)) | ||
108 | 1010 | self.assertEquals(list(values), [60]) | ||
109 | 1011 | |||
110 | 1005 | def test_find_values_by_column_name(self): | 1012 | def test_find_values_by_column_name(self): |
111 | 1013 | """ | ||
112 | 1014 | ResultSet.values() can accept column names which are mapped to | ||
113 | 1015 | columns in the find spec. | ||
114 | 1016 | """ | ||
115 | 1006 | result = self.store.find(Foo).order_by(Foo.id) | 1017 | result = self.store.find(Foo).order_by(Foo.id) |
116 | 1007 | values = result.values('id') | 1018 | values = result.values('id') |
117 | 1008 | self.assertEquals(list(values), [10, 20, 30]) | 1019 | self.assertEquals(list(values), [10, 20, 30]) |
118 | 1009 | values = result.values('title') | 1020 | values = result.values('title') |
119 | 1010 | self.assertEquals(list(values), ["Title 30", "Title 20", "Title 10"]) | 1021 | self.assertEquals(list(values), ["Title 30", "Title 20", "Title 10"]) |
122 | 1011 | # If more than one column in the result set has the same name, | 1022 | |
123 | 1012 | # the first will be chosen. | 1023 | def test_find_values_by_alias_name(self): |
124 | 1024 | """ | ||
125 | 1025 | Alias names can also be passed to ResultSet.values(). | ||
126 | 1026 | """ | ||
127 | 1027 | result = self.store.find(Alias(Foo.id, 'foo')).order_by(Foo.id) | ||
128 | 1028 | values = result.values('foo') | ||
129 | 1029 | self.assertEquals(list(values), [10, 20, 30]) | ||
130 | 1030 | |||
131 | 1031 | def test_find_values_by_alias_name_to_expression(self): | ||
132 | 1032 | """ | ||
133 | 1033 | Alias names can be passed to ResultSet.values(), even if the | ||
134 | 1034 | aliased column is actually an expression. | ||
135 | 1035 | """ | ||
136 | 1036 | result = self.store.find(Alias(Sum(Foo.id), 'foo')).order_by(Foo.id) | ||
137 | 1037 | values = result.values('foo') | ||
138 | 1038 | self.assertEquals(list(values), [60]) | ||
139 | 1039 | |||
140 | 1040 | def test_find_values_by_ambiguous_column_name(self): | ||
141 | 1041 | """ | ||
142 | 1042 | If more than one column in the find spec has the same name, | ||
143 | 1043 | FeatureError is raised. | ||
144 | 1044 | """ | ||
145 | 1013 | result = self.store.find( | 1045 | result = self.store.find( |
146 | 1014 | (Foo.id, FooValue.id), FooValue.foo_id == Foo.id) | 1046 | (Foo.id, FooValue.id), FooValue.foo_id == Foo.id) |
147 | 1015 | result = result.config(distinct=True).order_by(Foo.id) | 1047 | result = result.config(distinct=True).order_by(Foo.id) |
148 | 1016 | values = result.values('id') | 1048 | values = result.values('id') |
156 | 1017 | self.assertEquals(list(values), [10, 20]) | 1049 | self.assertRaises(FeatureError, list, values) |
150 | 1018 | # The name is only matched against columns, not aliases or | ||
151 | 1019 | # other expressions. | ||
152 | 1020 | result = self.store.find( | ||
153 | 1021 | (Alias(SQL('1'), 'id'), Foo.id)).order_by(Foo.id) | ||
154 | 1022 | values = result.values('id') | ||
155 | 1023 | self.assertEquals(list(values), [10, 20, 30]) | ||
157 | 1024 | 1050 | ||
158 | 1025 | def test_find_multiple_values(self): | 1051 | def test_find_multiple_values(self): |
159 | 1026 | result = self.store.find(Foo).order_by(Foo.id) | 1052 | result = self.store.find(Foo).order_by(Foo.id) |
160 | @@ -1031,12 +1057,22 @@ | |||
161 | 1031 | (30, "Title 10")]) | 1057 | (30, "Title 10")]) |
162 | 1032 | 1058 | ||
163 | 1033 | def test_find_multiple_values_by_column_name(self): | 1059 | def test_find_multiple_values_by_column_name(self): |
164 | 1060 | """ | ||
165 | 1061 | More than one column name can be given to ResultSet.values(); | ||
166 | 1062 | it will map them all to columns. | ||
167 | 1063 | """ | ||
168 | 1034 | result = self.store.find(Foo).order_by(Foo.id) | 1064 | result = self.store.find(Foo).order_by(Foo.id) |
169 | 1035 | values = result.values('id', 'title') | 1065 | values = result.values('id', 'title') |
170 | 1036 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] | 1066 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] |
171 | 1037 | self.assertEquals(list(values), expected) | 1067 | self.assertEquals(list(values), expected) |
173 | 1038 | # Columns and column names can be mixed. | 1068 | |
174 | 1069 | def test_find_multiple_values_by_column_and_column_name(self): | ||
175 | 1070 | """ | ||
176 | 1071 | Columns and column names can be mixed. | ||
177 | 1072 | """ | ||
178 | 1073 | result = self.store.find(Foo).order_by(Foo.id) | ||
179 | 1039 | values = result.values(Foo.id, 'title') | 1074 | values = result.values(Foo.id, 'title') |
180 | 1075 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] | ||
181 | 1040 | self.assertEquals(list(values), expected) | 1076 | self.assertEquals(list(values), expected) |
182 | 1041 | 1077 | ||
183 | 1042 | def test_find_values_with_no_arguments(self): | 1078 | def test_find_values_with_no_arguments(self): |
Gustavo Niemeyer (niemeyer) wrote : | # |
Gavin,
I don't quite see the motivation for this feature. Using the actual class attributes is how it works pretty much everywhere else in Storm, and doesn't look like a big burden when compared to the column names.
If you have a huge class name, you can easily do something like this:
C = MyHugeClassName
And then have exactly the same call length:
resultset.
vs.
resultset.
With the advantage that the former, in case of errors, will blow up as syntax errors early, rather than SQL exceptions.
The import fascist isn't a great reason to add support like this in Storm either. Having a result set in the code means you have access to the model objects. What would be the reason of preventing access to the object's class in a situation where you have the object and a *result set* (which allows one to change column values without any checks).
Then, there's a significant additional cost being introduced in a place which used to be pretty lightweight.
For these reasons, I don't feel like it's an improvement over how it works today.
Gavin Panella (allenap) wrote : | # |
> Gavin,
>
> I don't quite see the motivation for this feature. Using the actual class
> attributes is how it works pretty much everywhere else in Storm, and doesn't
> look like a big burden when compared to the column names.
This patch also lets you specify an aliased column, which could be an
expression rather than a plain column on a table. Perhaps there is a
better way of achieving this.
>
> If you have a huge class name, you can easily do something like this:
>
> C = MyHugeClassName
>
> And then have exactly the same call length:
>
> resultset.
>
> vs.
>
> resultset.
Brevity wasn't my concern in this patch :)
> With the advantage that the former, in case of errors, will blow up as syntax
> errors early, rather than SQL exceptions.
Actually, somewhat amusingly, you'll get the string for the invalid
column back instead. So:
list(
returns:
['frooty', 'frooty', 'frooty']
But that's possible to remedy.
>
> The import fascist isn't a great reason to add support like this in Storm
> either. Having a result set in the code means you have access to the model
> objects. What would be the reason of preventing access to the object's class
> in a situation where you have the object and a *result set* (which allows one
> to change column values without any checks).
In Launchpad result sets are often returned from methods called on a
secured utility, so browser code and script code does not have access
to the model class.
Any code that uses the objects materialized from a result set must
know the name of the attributes its interested in, so it makes sense
that it could ask for the values of those attributes across the rows
defined by the result set.
My use-case is a script with lots of transactions. A result set
defining the interesting rows is obtained early on from a secured
utility and is used in many separate transactions. Currently there can
be as many as ~16000 interesting rows. I want to avoid materializing
these rows into model objects until they're absolutely needed, because
it's slow and the transaction killer is merciless, but sometimes the
code does need one or two attributes from the whole set.
Anyway, that was my reasoning, but, as earlier, there may be a better
way to do it. I could, for example, add more methods on the secured
utility to return different information from the result set for
me. It's not really the way I'd like to structure the code but it only
offends my taste a little bit ;)
>
> Then, there's a significant additional cost being introduced in a place which
> used to be pretty lightweight.
I haven't measured it, but I guess that the cost is still small
compared to compiling the query and doing a round-trip to the
database.
>
> For these reasons, I don't feel like it's an improvement over how it works
> today.
Fair enough, I'm not blocked on this. Thanks for looking at it! I've
learnt a lot about Storm from doing this.
Unmerged revisions
- 362. By Gavin Panella
-
Make ResultSet.values() work with expressions.
- 361. By Gavin Panella
-
Add docstrings to new private methods.
- 360. By Gavin Panella
-
Fix some lint.
- 359. By Gavin Panella
-
Make the implementation of _get_column_
name_map( ) more readable and obvious. - 358. By Gavin Panella
-
Update the docstring for ResultSet.values().
- 357. By Gavin Panella
-
Raise an error when the column choice is ambiguous, copy the select before mutating it, and break up tests.
- 356. By Gavin Panella
-
ResultSet.values() can now accept column names as well as columns themselves.
Preview Diff
1 | === modified file 'storm/store.py' | |||
2 | --- storm/store.py 2010-02-10 11:29:39 +0000 | |||
3 | +++ storm/store.py 2010-04-22 10:53:24 +0000 | |||
4 | @@ -540,7 +540,6 @@ | |||
5 | 540 | else: | 540 | else: |
6 | 541 | 541 | ||
7 | 542 | cached_primary_vars = obj_info["primary_vars"] | 542 | cached_primary_vars = obj_info["primary_vars"] |
8 | 543 | primary_key_idx = cls_info.primary_key_idx | ||
9 | 544 | 543 | ||
10 | 545 | changes = self._get_changes_map(obj_info) | 544 | changes = self._get_changes_map(obj_info) |
11 | 546 | 545 | ||
12 | @@ -1246,29 +1245,63 @@ | |||
13 | 1246 | """Get the sum of all values in an expression.""" | 1245 | """Get the sum of all values in an expression.""" |
14 | 1247 | return self._aggregate(Sum, expr, expr) | 1246 | return self._aggregate(Sum, expr, expr) |
15 | 1248 | 1247 | ||
16 | 1248 | def _get_column_name_map(self): | ||
17 | 1249 | """Return a mapping of column name to lists of columns.""" | ||
18 | 1250 | columns, tables = self._find_spec.get_columns_and_tables() | ||
19 | 1251 | column_map = {} | ||
20 | 1252 | for column in columns: | ||
21 | 1253 | if isinstance(column, (Alias, Column)): | ||
22 | 1254 | if column.name in column_map: | ||
23 | 1255 | column_map[column.name].append(column) | ||
24 | 1256 | else: | ||
25 | 1257 | column_map[column.name] = [column] | ||
26 | 1258 | return column_map | ||
27 | 1259 | |||
28 | 1260 | def _map_column_names(self, columns): | ||
29 | 1261 | """Attempt to map column names to actual columns.""" | ||
30 | 1262 | column_map = self._get_column_name_map() | ||
31 | 1263 | for column in columns: | ||
32 | 1264 | if column in column_map: | ||
33 | 1265 | column_name, column_list = column, column_map[column] | ||
34 | 1266 | if len(column_list) != 1: | ||
35 | 1267 | raise FeatureError("Ambiguous column: %s" % column_name) | ||
36 | 1268 | [column] = column_list | ||
37 | 1269 | if isinstance(column, Alias): | ||
38 | 1270 | column = column.expr | ||
39 | 1271 | yield column | ||
40 | 1272 | |||
41 | 1249 | def values(self, *columns): | 1273 | def values(self, *columns): |
42 | 1250 | """Retrieve only the specified columns. | 1274 | """Retrieve only the specified columns. |
43 | 1251 | 1275 | ||
44 | 1252 | This does not load full objects from the database into Python. | 1276 | This does not load full objects from the database into Python. |
45 | 1253 | 1277 | ||
48 | 1254 | @param columns: One or more L{storm.expr.Column} objects whose | 1278 | @param columns: One or more L{storm.expr.Column} objects, or |
49 | 1255 | values will be fetched. | 1279 | column names, whose values will be fetched. If column |
50 | 1280 | names are given, each must refer unambiguously to a named | ||
51 | 1281 | column or alias in the result set. | ||
52 | 1256 | @return: An iterator of tuples of the values for each column | 1282 | @return: An iterator of tuples of the values for each column |
53 | 1257 | from each matching row in the database. | 1283 | from each matching row in the database. |
54 | 1258 | """ | 1284 | """ |
55 | 1259 | if not columns: | 1285 | if not columns: |
56 | 1260 | raise FeatureError("values() takes at least one column " | 1286 | raise FeatureError("values() takes at least one column " |
57 | 1261 | "as argument") | 1287 | "as argument") |
59 | 1262 | select = self._get_select() | 1288 | columns = list(self._map_column_names(columns)) |
60 | 1289 | # replace_columns() can lose ordering so it's not good here. | ||
61 | 1290 | select = copy(self._get_select()) | ||
62 | 1263 | select.columns = columns | 1291 | select.columns = columns |
63 | 1264 | result = self._store._connection.execute(select) | 1292 | result = self._store._connection.execute(select) |
66 | 1265 | if len(columns) == 1: | 1293 | variable_factories = [ |
67 | 1266 | variable = columns[0].variable_factory() | 1294 | getattr(column, 'variable_factory', Variable) |
68 | 1295 | for column in columns] | ||
69 | 1296 | variables = [ | ||
70 | 1297 | variable_factory() | ||
71 | 1298 | for variable_factory in variable_factories] | ||
72 | 1299 | if len(variables) == 1: | ||
73 | 1300 | [variable] = variables | ||
74 | 1267 | for values in result: | 1301 | for values in result: |
75 | 1268 | result.set_variable(variable, values[0]) | 1302 | result.set_variable(variable, values[0]) |
76 | 1269 | yield variable.get() | 1303 | yield variable.get() |
77 | 1270 | else: | 1304 | else: |
78 | 1271 | variables = [column.variable_factory() for column in columns] | ||
79 | 1272 | for values in result: | 1305 | for values in result: |
80 | 1273 | for variable, value in zip(variables, values): | 1306 | for variable, value in zip(variables, values): |
81 | 1274 | result.set_variable(variable, value) | 1307 | result.set_variable(variable, value) |
82 | @@ -1373,7 +1406,6 @@ | |||
83 | 1373 | def get_column(column): | 1406 | def get_column(column): |
84 | 1374 | return obj_info.variables[column].get() | 1407 | return obj_info.variables[column].get() |
85 | 1375 | objects = [] | 1408 | objects = [] |
86 | 1376 | cls = self._find_spec.default_cls_info.cls | ||
87 | 1377 | for obj_info in self._store._iter_alive(): | 1409 | for obj_info in self._store._iter_alive(): |
88 | 1378 | try: | 1410 | try: |
89 | 1379 | if (obj_info.cls_info is self._find_spec.default_cls_info and | 1411 | if (obj_info.cls_info is self._find_spec.default_cls_info and |
90 | 1380 | 1412 | ||
91 | === modified file 'tests/store/base.py' | |||
92 | --- tests/store/base.py 2010-04-16 07:12:13 +0000 | |||
93 | +++ tests/store/base.py 2010-04-22 10:53:24 +0000 | |||
94 | @@ -30,7 +30,8 @@ | |||
95 | 30 | from storm.properties import PropertyPublisherMeta, Decimal | 30 | from storm.properties import PropertyPublisherMeta, Decimal |
96 | 31 | from storm.variables import PickleVariable | 31 | from storm.variables import PickleVariable |
97 | 32 | from storm.expr import ( | 32 | from storm.expr import ( |
99 | 33 | Asc, Desc, Select, LeftJoin, SQL, Count, Sum, Avg, And, Or, Eq, Lower) | 33 | Alias, And, Asc, Avg, Count, Desc, Eq, LeftJoin, Lower, Or, SQL, Select, |
100 | 34 | Sum) | ||
101 | 34 | from storm.variables import Variable, UnicodeVariable, IntVariable | 35 | from storm.variables import Variable, UnicodeVariable, IntVariable |
102 | 35 | from storm.info import get_obj_info, ClassAlias | 36 | from storm.info import get_obj_info, ClassAlias |
103 | 36 | from storm.exceptions import * | 37 | from storm.exceptions import * |
104 | @@ -1001,6 +1002,52 @@ | |||
105 | 1001 | self.assertEquals([type(value) for value in values], | 1002 | self.assertEquals([type(value) for value in values], |
106 | 1002 | [unicode, unicode, unicode]) | 1003 | [unicode, unicode, unicode]) |
107 | 1003 | 1004 | ||
108 | 1005 | def test_find_values_with_expression(self): | ||
109 | 1006 | """ | ||
110 | 1007 | An expression can be passed to ResultSet.values(). | ||
111 | 1008 | """ | ||
112 | 1009 | values = self.store.find(Foo.id).values(Sum(Foo.id)) | ||
113 | 1010 | self.assertEquals(list(values), [60]) | ||
114 | 1011 | |||
115 | 1012 | def test_find_values_by_column_name(self): | ||
116 | 1013 | """ | ||
117 | 1014 | ResultSet.values() can accept column names which are mapped to | ||
118 | 1015 | columns in the find spec. | ||
119 | 1016 | """ | ||
120 | 1017 | result = self.store.find(Foo).order_by(Foo.id) | ||
121 | 1018 | values = result.values('id') | ||
122 | 1019 | self.assertEquals(list(values), [10, 20, 30]) | ||
123 | 1020 | values = result.values('title') | ||
124 | 1021 | self.assertEquals(list(values), ["Title 30", "Title 20", "Title 10"]) | ||
125 | 1022 | |||
126 | 1023 | def test_find_values_by_alias_name(self): | ||
127 | 1024 | """ | ||
128 | 1025 | Alias names can also be passed to ResultSet.values(). | ||
129 | 1026 | """ | ||
130 | 1027 | result = self.store.find(Alias(Foo.id, 'foo')).order_by(Foo.id) | ||
131 | 1028 | values = result.values('foo') | ||
132 | 1029 | self.assertEquals(list(values), [10, 20, 30]) | ||
133 | 1030 | |||
134 | 1031 | def test_find_values_by_alias_name_to_expression(self): | ||
135 | 1032 | """ | ||
136 | 1033 | Alias names can be passed to ResultSet.values(), even if the | ||
137 | 1034 | aliased column is actually an expression. | ||
138 | 1035 | """ | ||
139 | 1036 | result = self.store.find(Alias(Sum(Foo.id), 'foo')).order_by(Foo.id) | ||
140 | 1037 | values = result.values('foo') | ||
141 | 1038 | self.assertEquals(list(values), [60]) | ||
142 | 1039 | |||
143 | 1040 | def test_find_values_by_ambiguous_column_name(self): | ||
144 | 1041 | """ | ||
145 | 1042 | If more than one column in the find spec has the same name, | ||
146 | 1043 | FeatureError is raised. | ||
147 | 1044 | """ | ||
148 | 1045 | result = self.store.find( | ||
149 | 1046 | (Foo.id, FooValue.id), FooValue.foo_id == Foo.id) | ||
150 | 1047 | result = result.config(distinct=True).order_by(Foo.id) | ||
151 | 1048 | values = result.values('id') | ||
152 | 1049 | self.assertRaises(FeatureError, list, values) | ||
153 | 1050 | |||
154 | 1004 | def test_find_multiple_values(self): | 1051 | def test_find_multiple_values(self): |
155 | 1005 | result = self.store.find(Foo).order_by(Foo.id) | 1052 | result = self.store.find(Foo).order_by(Foo.id) |
156 | 1006 | values = result.values(Foo.id, Foo.title) | 1053 | values = result.values(Foo.id, Foo.title) |
157 | @@ -1009,6 +1056,25 @@ | |||
158 | 1009 | (20, "Title 20"), | 1056 | (20, "Title 20"), |
159 | 1010 | (30, "Title 10")]) | 1057 | (30, "Title 10")]) |
160 | 1011 | 1058 | ||
161 | 1059 | def test_find_multiple_values_by_column_name(self): | ||
162 | 1060 | """ | ||
163 | 1061 | More than one column name can be given to ResultSet.values(); | ||
164 | 1062 | it will map them all to columns. | ||
165 | 1063 | """ | ||
166 | 1064 | result = self.store.find(Foo).order_by(Foo.id) | ||
167 | 1065 | values = result.values('id', 'title') | ||
168 | 1066 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] | ||
169 | 1067 | self.assertEquals(list(values), expected) | ||
170 | 1068 | |||
171 | 1069 | def test_find_multiple_values_by_column_and_column_name(self): | ||
172 | 1070 | """ | ||
173 | 1071 | Columns and column names can be mixed. | ||
174 | 1072 | """ | ||
175 | 1073 | result = self.store.find(Foo).order_by(Foo.id) | ||
176 | 1074 | values = result.values(Foo.id, 'title') | ||
177 | 1075 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] | ||
178 | 1076 | self.assertEquals(list(values), expected) | ||
179 | 1077 | |||
180 | 1012 | def test_find_values_with_no_arguments(self): | 1078 | def test_find_values_with_no_arguments(self): |
181 | 1013 | result = self.store.find(Foo).order_by(Foo.id) | 1079 | result = self.store.find(Foo).order_by(Foo.id) |
182 | 1014 | self.assertRaises(FeatureError, result.values().next) | 1080 | self.assertRaises(FeatureError, result.values().next) |
Can you please file a bug and link this branch to it? All Storm
branches should have an associated bug.