Code review comment for lp:~allenap/storm/value-columns-by-name
- value-columns-by-name
- Merge into trunk
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
| 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 | else: |
| 6 | |
| 7 | cached_primary_vars = obj_info["primary_vars"] |
| 8 | - primary_key_idx = cls_info.primary_key_idx |
| 9 | |
| 10 | changes = self._get_changes_map(obj_info) |
| 11 | |
| 12 | @@ -1238,34 +1237,63 @@ |
| 13 | """Get the sum of all values in an expression.""" |
| 14 | return self._aggregate(Sum, expr, expr) |
| 15 | |
| 16 | + def _get_column_name_map(self): |
| 17 | + """Return a mapping of column name to lists of columns.""" |
| 18 | + columns, tables = self._find_spec.get_columns_and_tables() |
| 19 | + column_map = {} |
| 20 | + for column in columns: |
| 21 | + if isinstance(column, (Alias, Column)): |
| 22 | + if column.name in column_map: |
| 23 | + column_map[column.name].append(column) |
| 24 | + else: |
| 25 | + column_map[column.name] = [column] |
| 26 | + return column_map |
| 27 | + |
| 28 | + def _map_column_names(self, columns): |
| 29 | + """Attempt to map column names to actual columns.""" |
| 30 | + column_map = self._get_column_name_map() |
| 31 | + for column in columns: |
| 32 | + if column in column_map: |
| 33 | + column_name, column_list = column, column_map[column] |
| 34 | + if len(column_list) != 1: |
| 35 | + raise FeatureError("Ambiguous column: %s" % column_name) |
| 36 | + [column] = column_list |
| 37 | + if isinstance(column, Alias): |
| 38 | + column = column.expr |
| 39 | + yield column |
| 40 | + |
| 41 | def values(self, *columns): |
| 42 | """Retrieve only the specified columns. |
| 43 | |
| 44 | This does not load full objects from the database into Python. |
| 45 | |
| 46 | - @param columns: One or more L{storm.expr.Column} objects whose |
| 47 | - values will be fetched. |
| 48 | + @param columns: One or more L{storm.expr.Column} objects, or |
| 49 | + column names, whose values will be fetched. If column |
| 50 | + names are given, each must refer unambiguously to a named |
| 51 | + column or alias in the result set. |
| 52 | @return: An iterator of tuples of the values for each column |
| 53 | from each matching row in the database. |
| 54 | """ |
| 55 | if not columns: |
| 56 | raise FeatureError("values() takes at least one column " |
| 57 | "as argument") |
| 58 | - select = self._get_select() |
| 59 | - column_map = dict( |
| 60 | - (column.name, column) |
| 61 | - for column in reversed(select.columns) |
| 62 | - if isinstance(column, Column)) |
| 63 | - columns = [column_map.get(column, column) for column in columns] |
| 64 | + columns = list(self._map_column_names(columns)) |
| 65 | + # replace_columns() can lose ordering so it's not good here. |
| 66 | + select = copy(self._get_select()) |
| 67 | select.columns = columns |
| 68 | result = self._store._connection.execute(select) |
| 69 | - if len(columns) == 1: |
| 70 | - variable = columns[0].variable_factory() |
| 71 | + variable_factories = [ |
| 72 | + getattr(column, 'variable_factory', Variable) |
| 73 | + for column in columns] |
| 74 | + variables = [ |
| 75 | + variable_factory() |
| 76 | + for variable_factory in variable_factories] |
| 77 | + if len(variables) == 1: |
| 78 | + [variable] = variables |
| 79 | for values in result: |
| 80 | result.set_variable(variable, values[0]) |
| 81 | yield variable.get() |
| 82 | else: |
| 83 | - variables = [column.variable_factory() for column in columns] |
| 84 | for values in result: |
| 85 | for variable, value in zip(variables, values): |
| 86 | result.set_variable(variable, value) |
| 87 | @@ -1370,7 +1398,6 @@ |
| 88 | def get_column(column): |
| 89 | return obj_info.variables[column].get() |
| 90 | objects = [] |
| 91 | - cls = self._find_spec.default_cls_info.cls |
| 92 | for obj_info in self._store._iter_alive(): |
| 93 | try: |
| 94 | if (obj_info.cls_info is self._find_spec.default_cls_info and |
| 95 | |
| 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 | self.assertEquals([type(value) for value in values], |
| 101 | [unicode, unicode, unicode]) |
| 102 | |
| 103 | + def test_find_values_with_expression(self): |
| 104 | + """ |
| 105 | + An expression can be passed to ResultSet.values(). |
| 106 | + """ |
| 107 | + values = self.store.find(Foo.id).values(Sum(Foo.id)) |
| 108 | + self.assertEquals(list(values), [60]) |
| 109 | + |
| 110 | def test_find_values_by_column_name(self): |
| 111 | + """ |
| 112 | + ResultSet.values() can accept column names which are mapped to |
| 113 | + columns in the find spec. |
| 114 | + """ |
| 115 | result = self.store.find(Foo).order_by(Foo.id) |
| 116 | values = result.values('id') |
| 117 | self.assertEquals(list(values), [10, 20, 30]) |
| 118 | values = result.values('title') |
| 119 | self.assertEquals(list(values), ["Title 30", "Title 20", "Title 10"]) |
| 120 | - # If more than one column in the result set has the same name, |
| 121 | - # the first will be chosen. |
| 122 | + |
| 123 | + def test_find_values_by_alias_name(self): |
| 124 | + """ |
| 125 | + Alias names can also be passed to ResultSet.values(). |
| 126 | + """ |
| 127 | + result = self.store.find(Alias(Foo.id, 'foo')).order_by(Foo.id) |
| 128 | + values = result.values('foo') |
| 129 | + self.assertEquals(list(values), [10, 20, 30]) |
| 130 | + |
| 131 | + def test_find_values_by_alias_name_to_expression(self): |
| 132 | + """ |
| 133 | + Alias names can be passed to ResultSet.values(), even if the |
| 134 | + aliased column is actually an expression. |
| 135 | + """ |
| 136 | + result = self.store.find(Alias(Sum(Foo.id), 'foo')).order_by(Foo.id) |
| 137 | + values = result.values('foo') |
| 138 | + self.assertEquals(list(values), [60]) |
| 139 | + |
| 140 | + def test_find_values_by_ambiguous_column_name(self): |
| 141 | + """ |
| 142 | + If more than one column in the find spec has the same name, |
| 143 | + FeatureError is raised. |
| 144 | + """ |
| 145 | result = self.store.find( |
| 146 | (Foo.id, FooValue.id), FooValue.foo_id == Foo.id) |
| 147 | result = result.config(distinct=True).order_by(Foo.id) |
| 148 | values = result.values('id') |
| 149 | - self.assertEquals(list(values), [10, 20]) |
| 150 | - # The name is only matched against columns, not aliases or |
| 151 | - # other expressions. |
| 152 | - result = self.store.find( |
| 153 | - (Alias(SQL('1'), 'id'), Foo.id)).order_by(Foo.id) |
| 154 | - values = result.values('id') |
| 155 | - self.assertEquals(list(values), [10, 20, 30]) |
| 156 | + self.assertRaises(FeatureError, list, values) |
| 157 | |
| 158 | def test_find_multiple_values(self): |
| 159 | result = self.store.find(Foo).order_by(Foo.id) |
| 160 | @@ -1031,12 +1057,22 @@ |
| 161 | (30, "Title 10")]) |
| 162 | |
| 163 | def test_find_multiple_values_by_column_name(self): |
| 164 | + """ |
| 165 | + More than one column name can be given to ResultSet.values(); |
| 166 | + it will map them all to columns. |
| 167 | + """ |
| 168 | result = self.store.find(Foo).order_by(Foo.id) |
| 169 | values = result.values('id', 'title') |
| 170 | expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] |
| 171 | self.assertEquals(list(values), expected) |
| 172 | - # Columns and column names can be mixed. |
| 173 | + |
| 174 | + def test_find_multiple_values_by_column_and_column_name(self): |
| 175 | + """ |
| 176 | + Columns and column names can be mixed. |
| 177 | + """ |
| 178 | + result = self.store.find(Foo).order_by(Foo.id) |
| 179 | values = result.values(Foo.id, 'title') |
| 180 | + expected = [(10, "Title 30"), (20, "Title 20"), (30, "Title 10")] |
| 181 | self.assertEquals(list(values), expected) |
| 182 | |
| 183 | def test_find_values_with_no_arguments(self): |
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()?