From 5ea20a11a9d59eab637a23e407a8489280704b7a Mon Sep 17 00:00:00 2001 From: M1ha Date: Tue, 10 Oct 2017 12:23:31 +0500 Subject: [PATCH 1/3] Added tests and resolved https://github.com/Infinidat/infi.clickhouse_orm/issues/47 --- src/infi/clickhouse_orm/fields.py | 31 +++++++++++++------------- src/infi/clickhouse_orm/migrations.py | 30 +++++++++++++++++++------ tests/sample_migrations/0012.py | 7 ++++++ tests/test_migrations.py | 32 +++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 22 deletions(-) create mode 100644 tests/sample_migrations/0012.py diff --git a/src/infi/clickhouse_orm/fields.py b/src/infi/clickhouse_orm/fields.py index 3e3207a..ef91b91 100644 --- a/src/infi/clickhouse_orm/fields.py +++ b/src/infi/clickhouse_orm/fields.py @@ -61,19 +61,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 @@ -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) diff --git a/src/infi/clickhouse_orm/migrations.py b/src/infi/clickhouse_orm/migrations.py index a7843a7..3149f0f 100644 --- a/src/infi/clickhouse_orm/migrations.py +++ b/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) - 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 new file mode 100644 index 0000000..14553f3 --- /dev/null +++ b/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) +] diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 7e31c84..29bff7d 100644 --- a/tests/test_migrations.py +++ b/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() From 7712b3fee0c1b03ba97e0a62403c790d63325365 Mon Sep 17 00:00:00 2001 From: Itai Shirav Date: Mon, 30 Oct 2017 15:49:53 +0200 Subject: [PATCH 2/3] Documentation --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21613ee..1adc4ec 100644 --- a/CHANGELOG.md +++ b/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 ------ From c6905cbc98e757c4b7b70c4ea783b4155536f2cc Mon Sep 17 00:00:00 2001 From: Itai Shirav Date: Mon, 30 Oct 2017 15:51:10 +0200 Subject: [PATCH 3/3] Missing brackets --- tests/sample_migrations/0012.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/sample_migrations/0012.py b/tests/sample_migrations/0012.py index 20fc758..1dfe3bf 100644 --- a/tests/sample_migrations/0012.py +++ b/tests/sample_migrations/0012.py @@ -6,3 +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') ", ]) +]