From 1bd3e63cd48b4ef76be78b72322feaa7e9925db3 Mon Sep 17 00:00:00 2001 From: Itai Shirav Date: Thu, 10 May 2018 15:24:38 +0300 Subject: [PATCH] minor improvements in error handling and testing --- src/infi/clickhouse_orm/database.py | 3 ++- src/infi/clickhouse_orm/engines.py | 3 ++- src/infi/clickhouse_orm/fields.py | 2 +- tests/base_test_with_data.py | 4 ++-- tests/test_database.py | 16 +++++++++++++++- tests/test_engines.py | 6 ++++++ 6 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/infi/clickhouse_orm/database.py b/src/infi/clickhouse_orm/database.py index aa53b29..0ef6b57 100644 --- a/src/infi/clickhouse_orm/database.py +++ b/src/infi/clickhouse_orm/database.py @@ -116,9 +116,10 @@ class Database(object): ''' Creates a table for the given model class, if it does not exist already. ''' - # TODO check that model has an engine if model_class.system: raise DatabaseException("You can't create system table") + if getattr(model_class, 'engine') is None: + raise DatabaseException("%s class must define an engine" % model_class.__name__) self._send(model_class.create_table_sql(self)) def drop_table(self, model_class): diff --git a/src/infi/clickhouse_orm/engines.py b/src/infi/clickhouse_orm/engines.py index 1acd73d..ea6d3f4 100644 --- a/src/infi/clickhouse_orm/engines.py +++ b/src/infi/clickhouse_orm/engines.py @@ -40,6 +40,8 @@ class MergeTree(Engine): assert date_col is None or isinstance(date_col, six.string_types), 'date_col must be string if present' assert partition_key is None or type(partition_key) in (list, tuple),\ 'partition_key must be tuple or list if present' + assert (replica_table_path is None) == (replica_name == None), \ + 'both replica_table_path and replica_name must be specified' # These values conflict with each other (old and new syntax of table engines. # So let's control only one of them is given. @@ -52,7 +54,6 @@ class MergeTree(Engine): self.index_granularity = index_granularity self.replica_table_path = replica_table_path self.replica_name = replica_name - # TODO verify that both replica fields are either present or missing # I changed field name for new reality and syntax @property diff --git a/src/infi/clickhouse_orm/fields.py b/src/infi/clickhouse_orm/fields.py index 5449e9d..fc40790 100644 --- a/src/infi/clickhouse_orm/fields.py +++ b/src/infi/clickhouse_orm/fields.py @@ -39,7 +39,7 @@ class Field(object): data can't be converted. Returns the converted value. Subclasses should override this. The timezone_in_use parameter should be consulted when parsing datetime fields. ''' - return value + return value # pragma: no cover def validate(self, value): ''' diff --git a/tests/base_test_with_data.py b/tests/base_test_with_data.py index 352d3d3..90a328d 100644 --- a/tests/base_test_with_data.py +++ b/tests/base_test_with_data.py @@ -21,8 +21,8 @@ class TestCaseWithData(unittest.TestCase): self.database.drop_table(Person) self.database.drop_database() - def _insert_and_check(self, data, count): - self.database.insert(data) + def _insert_and_check(self, data, count, batch_size=1000): + self.database.insert(data, batch_size=batch_size) self.assertEquals(count, self.database.count(Person)) for instance in data: self.assertEquals(self.database, instance.get_database()) diff --git a/tests/test_database.py b/tests/test_database.py index 8e7c5c4..77bf1f0 100644 --- a/tests/test_database.py +++ b/tests/test_database.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals import unittest -from infi.clickhouse_orm.database import ServerError +from infi.clickhouse_orm.database import ServerError, DatabaseException from .base_test_with_data import * @@ -20,6 +20,12 @@ class DatabaseTestCase(TestCaseWithData): def test_insert__empty(self): self._insert_and_check([], 0) + def test_insert__small_batches(self): + self._insert_and_check(self._sample_data(), len(data), batch_size=10) + + def test_insert__medium_batches(self): + self._insert_and_check(self._sample_data(), len(data), batch_size=100) + def test_count(self): self.database.insert(self._sample_data()) self.assertEquals(self.database.count(Person), 100) @@ -150,3 +156,11 @@ class DatabaseTestCase(TestCaseWithData): def test_preexisting_db(self): db = Database(self.database.db_name, autocreate=False) db.count(Person) + + def test_missing_engine(self): + class EnginelessModel(Model): + float_field = Float32Field() + with self.assertRaises(DatabaseException) as cm: + self.database.create_table(EnginelessModel) + self.assertEqual(cm.exception.message, 'EnginelessModel class must define an engine') + diff --git a/tests/test_engines.py b/tests/test_engines.py index 88266cf..85966df 100644 --- a/tests/test_engines.py +++ b/tests/test_engines.py @@ -51,6 +51,12 @@ class EnginesTestCase(_EnginesHelperTestCase): expected = "ReplicatedMergeTree('/clickhouse/tables/{layer}-{shard}/hits', '{replica}', date, (date, event_id, event_group), 8192)" self.assertEquals(engine.create_table_sql(self.database), expected) + def test_replicated_merge_tree_incomplete(self): + with self.assertRaises(AssertionError): + MergeTree('date', ('date', 'event_id', 'event_group'), replica_table_path='/clickhouse/tables/{layer}-{shard}/hits') + with self.assertRaises(AssertionError): + MergeTree('date', ('date', 'event_id', 'event_group'), replica_name='{replica}') + def test_collapsing_merge_tree(self): class TestModel(SampleModel): engine = CollapsingMergeTree('date', ('date', 'event_id', 'event_group'), 'event_version')