Added tests and resolved https://github.com/Infinidat/infi.clickhouse_orm/issues/47

src/infi/clickhouse_orm/fields.py

@@ -61,17 +61,18 @@ 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 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)
-        elif with_default:
+            else:
                 default = self.to_db_string(self.default)
                 return '%s DEFAULT %s' % (self.db_type, default)
         else:
@@ -295,10 +296,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
@@ -357,9 +358,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):
@@ -387,6 +388,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

@@ -57,13 +57,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:
@@ -72,14 +77,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)
+
+            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

@@ -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

@@ -80,6 +80,14 @@ class MigrationsTestCase(unittest.TestCase):
         self.assertEquals(self.getTableFields(Model4), [('date', 'Date'), ('f3', 'DateTime'), ('f2', 'String')])
         self.assertEquals(self.getTableFields(Model4Buffer), [('date', 'Date'), ('f3', 'DateTime'), ('f2', 'String')])
 
+        self.database.migrate('tests.sample_migrations', 12)
+        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
 
@@ -160,6 +168,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')
@@ -171,6 +191,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()