Merge branch 'carrotquest-migrate_materialized_fields' into develop

CHANGELOG.md

@@ -7,6 +7,8 @@ Unreleased
 - Added RunPython and RunSQL migrations (M1hacka)
 - Allow ISO-formatted datetime values (tsionyx)
 - Show field name in error message when invalid value assigned (tsionyx)
+- Bug fix: select query fails when query contains '$' symbol (M1hacka)
+- Prevent problems with AlterTable migrations related to field order (M1hacka)
 
 v0.9.7
 ------

src/infi/clickhouse_orm/fields.py

@@ -62,19 +62,20 @@ class Field(object):
         '''
         return escape(value, quote)
 
-    def get_sql(self, with_default=True):
+    def get_sql(self, with_default_expression=True):
         '''
         Returns an SQL expression describing the field (e.g. for CREATE TABLE).
-        :param with_default: If True, adds default value to sql.
+        :param with_default_expression: If True, adds default value to sql.
             It doesn't affect fields with alias and materialized values.
         '''
-        if self.alias:
-            return '%s ALIAS %s' % (self.db_type, self.alias)
-        elif self.materialized:
-            return '%s MATERIALIZED %s' % (self.db_type, self.materialized)
-        elif with_default:
-            default = self.to_db_string(self.default)
-            return '%s DEFAULT %s' % (self.db_type, default)
+        if with_default_expression:
+            if self.alias:
+                return '%s ALIAS %s' % (self.db_type, self.alias)
+            elif self.materialized:
+                return '%s MATERIALIZED %s' % (self.db_type, self.materialized)
+            else:
+                default = self.to_db_string(self.default)
+                return '%s DEFAULT %s' % (self.db_type, default)
         else:
             return self.db_type
 
@@ -304,10 +305,10 @@ class BaseEnumField(Field):
     def to_db_string(self, value, quote=True):
         return escape(value.name, quote)
 
-    def get_sql(self, with_default=True):
+    def get_sql(self, with_default_expression=True):
         values = ['%s = %d' % (escape(item.name), item.value) for item in self.enum_cls]
         sql = '%s(%s)' % (self.db_type, ' ,'.join(values))
-        if with_default:
+        if with_default_expression:
             default = self.to_db_string(self.default)
             sql = '%s DEFAULT %s' % (sql, default)
         return sql
@@ -366,9 +367,9 @@ class ArrayField(Field):
         array = [self.inner_field.to_db_string(v, quote=True) for v in value]
         return '[' + comma_join(array) + ']'
 
-    def get_sql(self, with_default=True):
+    def get_sql(self, with_default_expression=True):
         from .utils import escape
-        return 'Array(%s)' % self.inner_field.get_sql(with_default=False)
+        return 'Array(%s)' % self.inner_field.get_sql(with_default_expression=False)
 
 
 class NullableField(Field):
@@ -396,6 +397,6 @@ class NullableField(Field):
             return '\\N'
         return self.inner_field.to_db_string(value, quote=quote)
 
-    def get_sql(self, with_default=True):
+    def get_sql(self, with_default_expression=True):
         from .utils import escape
-        return 'Nullable(%s)' % self.inner_field.get_sql(with_default=False)
+        return 'Nullable(%s)' % self.inner_field.get_sql(with_default_expression=False)

src/infi/clickhouse_orm/migrations.py

@@ -59,13 +59,18 @@ class AlterTable(Operation):
 
     def apply(self, database):
         logger.info('    Alter table %s', self.model_class.table_name())
+
+        # Note that MATERIALIZED and ALIAS fields are always at the end of the DESC,
+        # ADD COLUMN ... AFTER doesn't affect it
         table_fields = dict(self._get_table_fields(database))
+
         # Identify fields that were deleted from the model
         deleted_fields = set(table_fields.keys()) - set(name for name, field in self.model_class._fields)
         for name in deleted_fields:
             logger.info('        Drop column %s', name)
             self._alter_table(database, 'DROP COLUMN %s' % name)
             del table_fields[name]
+
         # Identify fields that were added to the model
         prev_name = None
         for name, field in self.model_class._fields:
@@ -74,14 +79,25 @@ class AlterTable(Operation):
                 assert prev_name, 'Cannot add a column to the beginning of the table'
                 cmd = 'ADD COLUMN %s %s AFTER %s' % (name, field.get_sql(), prev_name)
                 self._alter_table(database, cmd)
-            prev_name = name
+
+            if not field.materialized and not field.alias:
+                # ALIAS and MATERIALIZED fields are not stored in the database, and raise DatabaseError
+                # (no AFTER column). So we will skip them
+                prev_name = name
+
         # Identify fields whose type was changed
-        model_fields = [(name, field.get_sql(with_default=False)) for name, field in self.model_class._fields]
-        for model_field, table_field in zip(model_fields, self._get_table_fields(database)):
-            assert model_field[0] == table_field[0], 'Model fields and table columns in disagreement'
-            if model_field[1] != table_field[1]:
-                logger.info('        Change type of column %s from %s to %s', table_field[0], table_field[1], model_field[1])
-                self._alter_table(database, 'MODIFY COLUMN %s %s' % model_field)
+        # The order of class attributes can be changed any time, so we can't count on it
+        # Secondly, MATERIALIZED and ALIAS fields are always at the end of the DESC, so we can't expect them to save
+        # attribute position. Watch https://github.com/Infinidat/infi.clickhouse_orm/issues/47
+        model_fields = {name: field.get_sql(with_default_expression=False) for name, field in self.model_class._fields}
+        for field_name, field_sql in self._get_table_fields(database):
+            # All fields must have been created and dropped by this moment
+            assert field_name in model_fields, 'Model fields and table columns in disagreement'
+
+            if field_sql != model_fields[field_name]:
+                logger.info('        Change type of column %s from %s to %s', field_name, field_sql,
+                            model_fields[field_name])
+                self._alter_table(database, 'MODIFY COLUMN %s %s' % (field_name, model_fields[field_name]))
 
 
 class AlterTableWithBuffer(Operation):

tests/sample_migrations/0012.py

@@ -6,4 +6,4 @@ operations = [
         "INSERT INTO `mig` (date, f1, f3, f4) VALUES ('2016-01-02', 2, 2, 'test2') ",
         "INSERT INTO `mig` (date, f1, f3, f4) VALUES ('2016-01-03', 3, 3, 'test3') ",
     ])
-]
+]

tests/sample_migrations/0014.py

@@ -0,0 +1,7 @@
+from infi.clickhouse_orm import migrations
+from ..test_migrations import *
+
+operations = [
+    migrations.AlterTable(MaterializedModel1),
+    migrations.AlterTable(AliasModel1)
+]

tests/test_migrations.py

@@ -90,6 +90,14 @@ class MigrationsTestCase(unittest.TestCase):
         data = [item.f1 for item in self.database.select('SELECT f1 FROM $table ORDER BY f1', model_class=Model3)]
         self.assertListEqual(data, [1, 2, 3, 4])
 
+        self.database.migrate('tests.sample_migrations', 14)
+        self.assertTrue(self.tableExists(MaterializedModel1))
+        self.assertEquals(self.getTableFields(MaterializedModel1),
+                          [('date_time', "DateTime"), ('int_field', 'Int8'), ('date', 'Date')])
+        self.assertTrue(self.tableExists(AliasModel1))
+        self.assertEquals(self.getTableFields(AliasModel1),
+                          [('date', 'Date'), ('int_field', 'Int8'), ('date_alias', "Date")])
+
 
 # Several different models with the same table name, to simulate a table that changes over time
 
@@ -170,6 +178,18 @@ class MaterializedModel(Model):
         return 'materalized_date'
 
 
+class MaterializedModel1(Model):
+    date_time = DateTimeField()
+    date = DateField(materialized='toDate(date_time)')
+    int_field = Int8Field()
+
+    engine = MergeTree('date', ('date',))
+
+    @classmethod
+    def table_name(cls):
+        return 'materalized_date'
+
+
 class AliasModel(Model):
     date = DateField()
     date_alias = DateField(alias='date')
@@ -181,6 +201,18 @@ class AliasModel(Model):
         return 'alias_date'
 
 
+class AliasModel1(Model):
+    date = DateField()
+    date_alias = DateField(alias='date')
+    int_field = Int8Field()
+
+    engine = MergeTree('date', ('date',))
+
+    @classmethod
+    def table_name(cls):
+        return 'alias_date'
+
+
 class Model4(Model):
 
     date = DateField()