diff --git a/src/infi/clickhouse_orm/fields.py b/src/infi/clickhouse_orm/fields.py index d1a8d4e..5449e9d 100644 --- a/src/infi/clickhouse_orm/fields.py +++ b/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) diff --git a/src/infi/clickhouse_orm/migrations.py b/src/infi/clickhouse_orm/migrations.py index da2f1b8..324db74 100644 --- a/src/infi/clickhouse_orm/migrations.py +++ b/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): diff --git a/tests/sample_migrations/0012.py b/tests/sample_migrations/0012.py index 7a688cf..20fc758 100644 --- a/tests/sample_migrations/0012.py +++ b/tests/sample_migrations/0012.py @@ -6,4 +6,3 @@ 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') ", ]) -] \ No newline at end of file diff --git a/tests/sample_migrations/0014.py b/tests/sample_migrations/0014.py new file mode 100644 index 0000000..14553f3 --- /dev/null +++ b/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) +] diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 1b6093d..d42879b 100644 --- a/tests/test_migrations.py +++ b/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()