From 79689444bc11e63187383e9274b299e74aea5709 Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Mon, 1 Jul 2024 14:28:24 +0300 Subject: [PATCH 1/5] use catalogs in database service --- src/firebolt/service/V2/database.py | 57 ++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/src/firebolt/service/V2/database.py b/src/firebolt/service/V2/database.py index 0f72f4fbcc..da8ae8b305 100644 --- a/src/firebolt/service/V2/database.py +++ b/src/firebolt/service/V2/database.py @@ -1,28 +1,16 @@ import logging -from typing import List, Optional, Union +from functools import cached_property +from typing import List, Optional, Tuple, Union from firebolt.model.V2.database import Database from firebolt.model.V2.engine import Engine from firebolt.service.V2.base import BaseService -from firebolt.utils.exception import DatabaseNotFoundError +from firebolt.utils.exception import DatabaseNotFoundError, OperationalError logger = logging.getLogger(__name__) class DatabaseService(BaseService): - DB_FIELDS = ( - "database_name", - "description", - "region", - "uncompressed_size", - "compressed_size", - "attached_engines", - "created_on", - "created_by", - "errors", - ) - GET_SQL = f"SELECT {', '.join(DB_FIELDS)} FROM information_schema.databases" - GET_BY_NAME_SQL = GET_SQL + " WHERE database_name=?" GET_WHERE_SQL = " WHERE " CREATE_PREFIX_SQL = 'CREATE DATABASE {}"{}"' @@ -33,9 +21,44 @@ class DatabaseService(BaseService): "attached_engine_name_contains", ] + @cached_property + def catalog_name(self) -> str: + query = "SELECT count(*) FROM information_schema.catalogs" + with self._connection.cursor() as c: + try: + c.execute(query) + except OperationalError: + return "database" + return "catalog" + + @cached_property + def db_fields(self) -> Tuple[str, ...]: + return ( + f"{self.catalog_name}_name", + "description", + "region", + "uncompressed_size", + "compressed_size", + "attached_engines", + "created_on", + "created_by", + "errors", + ) + + @cached_property + def get_sql(self) -> str: + return ( + f"SELECT {', '.join(self.db_fields)} " + f"FROM information_schema.{self.catalog_name}s" + ) + + @cached_property + def get_by_name_sql(self) -> str: + return self.get_sql + " WHERE database_name=?" + def _get_dict(self, name: str) -> dict: with self._connection.cursor() as c: - count = c.execute(self.GET_BY_NAME_SQL, (name,)) + count = c.execute(self.get_by_name_sql, (name,)) if count == 0: raise DatabaseNotFoundError(name) return { @@ -69,7 +92,7 @@ def get_many( Returns: A list of databases matching the filters """ - sql = self.GET_SQL + sql = self.get_sql parameters = [] disallowed_parameters = [ name From aabec94bb3228c22f8ac64e13c8dad38da327f8c Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Mon, 1 Jul 2024 15:02:46 +0300 Subject: [PATCH 2/5] unit tests --- src/firebolt/model/V2/database.py | 2 +- tests/unit/service/test_database.py | 29 ++++++++++++++++++++++++++++- tests/unit/service/test_engine.py | 4 ++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/firebolt/model/V2/database.py b/src/firebolt/model/V2/database.py index 34e546b296..d4c4b4dec9 100644 --- a/src/firebolt/model/V2/database.py +++ b/src/firebolt/model/V2/database.py @@ -33,7 +33,7 @@ class Database(FireboltBaseModel): _service: DatabaseService = field(repr=False, compare=False) # required - name: str = field(metadata={"db_name": "database_name"}) + name: str = field(metadata={"db_name": ("database_name", "catalog_name")}) description: str = field() region: str = field() data_size_full: int = field(metadata={"db_name": "uncompressed_size"}) diff --git a/tests/unit/service/test_database.py b/tests/unit/service/test_database.py index 2b476e36af..8690fa89f1 100644 --- a/tests/unit/service/test_database.py +++ b/tests/unit/service/test_database.py @@ -7,7 +7,7 @@ from firebolt.model.V2.database import Database from firebolt.model.V2.engine import Engine from firebolt.service.manager import ResourceManager -from firebolt.utils.exception import AttachedEngineInUseError +from firebolt.utils.exception import AttachedEngineInUseError, OperationalError def test_database_create( @@ -179,3 +179,30 @@ def test_database_update_with_attached_engine_in_use( with raises(AttachedEngineInUseError): mock_database.update(description="new description") + + +def test_database_catalog_name( + httpx_mock: HTTPXMock, + select_one_query_callback: Callable, + system_engine_no_db_query_url: str, + resource_manager: ResourceManager, +): + httpx_mock.add_callback( + select_one_query_callback, url=system_engine_no_db_query_url + ) + + assert resource_manager.databases.catalog_name == "catalog" + + +def test_database_catalog_name_backup( + httpx_mock: HTTPXMock, + select_one_query_callback: Callable, + system_engine_no_db_query_url: str, + resource_manager: ResourceManager, +): + def failure_callback(*args, **kwargs) -> Response: + raise OperationalError("Catalogs don't exist") + + httpx_mock.add_callback(failure_callback, url=system_engine_no_db_query_url) + + assert resource_manager.databases.catalog_name == "database" diff --git a/tests/unit/service/test_engine.py b/tests/unit/service/test_engine.py index 8e16deb3b0..d29b351775 100644 --- a/tests/unit/service/test_engine.py +++ b/tests/unit/service/test_engine.py @@ -94,7 +94,11 @@ def test_attach_to_database( database_get_callback: Callable, attach_engine_to_db_callback: Callable, system_engine_no_db_query_url: str, + select_one_query_callback: Callable, ): + httpx_mock.add_callback( + select_one_query_callback, url=system_engine_no_db_query_url + ) httpx_mock.add_callback(database_get_callback, url=system_engine_no_db_query_url) httpx_mock.add_callback(get_engine_callback, url=system_engine_no_db_query_url) httpx_mock.add_callback( From 330c8b463c6073c03086b0997fc9aaf6f11d654b Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Tue, 2 Jul 2024 11:44:07 +0300 Subject: [PATCH 3/5] remove region column --- src/firebolt/service/V2/database.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/firebolt/service/V2/database.py b/src/firebolt/service/V2/database.py index da8ae8b305..263cb7648f 100644 --- a/src/firebolt/service/V2/database.py +++ b/src/firebolt/service/V2/database.py @@ -36,7 +36,6 @@ def db_fields(self) -> Tuple[str, ...]: return ( f"{self.catalog_name}_name", "description", - "region", "uncompressed_size", "compressed_size", "attached_engines", From 2dc133a46af9121eaa5d07220ca1a48e7239b3a9 Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Tue, 2 Jul 2024 14:30:43 +0300 Subject: [PATCH 4/5] always use catalogs --- src/firebolt/model/V2/database.py | 13 ++--- src/firebolt/service/V2/database.py | 53 +++++-------------- .../resource_manager/V2/test_database.py | 18 +------ tests/unit/service/conftest.py | 10 ---- tests/unit/service/test_database.py | 29 +--------- tests/unit/service/test_engine.py | 3 -- 6 files changed, 19 insertions(+), 107 deletions(-) diff --git a/src/firebolt/model/V2/database.py b/src/firebolt/model/V2/database.py index d4c4b4dec9..ac215f7173 100644 --- a/src/firebolt/model/V2/database.py +++ b/src/firebolt/model/V2/database.py @@ -33,17 +33,10 @@ class Database(FireboltBaseModel): _service: DatabaseService = field(repr=False, compare=False) # required - name: str = field(metadata={"db_name": ("database_name", "catalog_name")}) + name: str = field(metadata={"db_name": "catalog_name"}) description: str = field() - region: str = field() - data_size_full: int = field(metadata={"db_name": "uncompressed_size"}) - data_size_compressed: int = field(metadata={"db_name": "compressed_size"}) - _attached_engine_names: str = field( - repr=False, metadata={"db_name": "attached_engines"}, compare=False - ) - create_time: datetime = field(metadata={"db_name": "created_on"}) - create_actor: str = field(metadata={"db_name": "created_by"}) - _errors: str = field(repr=False, metadata={"db_name": "errors"}) + create_time: datetime = field(metadata={"db_name": "created"}) + create_actor: str = field(metadata={"db_name": "catalog_owner"}) def get_attached_engines(self) -> List[Engine]: """Get a list of engines that are attached to this database.""" diff --git a/src/firebolt/service/V2/database.py b/src/firebolt/service/V2/database.py index 263cb7648f..de6ba58834 100644 --- a/src/firebolt/service/V2/database.py +++ b/src/firebolt/service/V2/database.py @@ -1,16 +1,23 @@ import logging -from functools import cached_property -from typing import List, Optional, Tuple, Union +from typing import List, Optional, Union from firebolt.model.V2.database import Database from firebolt.model.V2.engine import Engine from firebolt.service.V2.base import BaseService -from firebolt.utils.exception import DatabaseNotFoundError, OperationalError +from firebolt.utils.exception import DatabaseNotFoundError logger = logging.getLogger(__name__) class DatabaseService(BaseService): + DB_FIELDS = ( + "catalog_name", + "description", + "created", + "catalog_owner", + ) + GET_SQL = f"SELECT {', '.join(DB_FIELDS)} FROM information_schema.catalogs" + GET_BY_NAME_SQL = GET_SQL + " WHERE catalog_name=?" GET_WHERE_SQL = " WHERE " CREATE_PREFIX_SQL = 'CREATE DATABASE {}"{}"' @@ -21,43 +28,9 @@ class DatabaseService(BaseService): "attached_engine_name_contains", ] - @cached_property - def catalog_name(self) -> str: - query = "SELECT count(*) FROM information_schema.catalogs" - with self._connection.cursor() as c: - try: - c.execute(query) - except OperationalError: - return "database" - return "catalog" - - @cached_property - def db_fields(self) -> Tuple[str, ...]: - return ( - f"{self.catalog_name}_name", - "description", - "uncompressed_size", - "compressed_size", - "attached_engines", - "created_on", - "created_by", - "errors", - ) - - @cached_property - def get_sql(self) -> str: - return ( - f"SELECT {', '.join(self.db_fields)} " - f"FROM information_schema.{self.catalog_name}s" - ) - - @cached_property - def get_by_name_sql(self) -> str: - return self.get_sql + " WHERE database_name=?" - def _get_dict(self, name: str) -> dict: with self._connection.cursor() as c: - count = c.execute(self.get_by_name_sql, (name,)) + count = c.execute(self.GET_BY_NAME_SQL, (name,)) if count == 0: raise DatabaseNotFoundError(name) return { @@ -91,7 +64,7 @@ def get_many( Returns: A list of databases matching the filters """ - sql = self.get_sql + sql = self.GET_SQL parameters = [] disallowed_parameters = [ name @@ -108,7 +81,7 @@ def get_many( ) if name_contains: - sql += " WHERE database_name like ?" + sql += " WHERE catalog_name like ?" parameters.append(f"%{name_contains}%") with self._connection.cursor() as c: diff --git a/tests/integration/resource_manager/V2/test_database.py b/tests/integration/resource_manager/V2/test_database.py index 4cfe73a7f8..10dcaca76a 100644 --- a/tests/integration/resource_manager/V2/test_database.py +++ b/tests/integration/resource_manager/V2/test_database.py @@ -31,19 +31,5 @@ def test_databases_get_many( with raises(ValueError): rm.databases.get_many(attached_engine_name_contains=engine_name) - region = [db for db in databases if db.name == database_name][0].region - - # get all databases, with region_eq - databases = rm.databases.get_many(region_eq=region) - assert len(databases) > 0 - assert database_name in {db.name for db in databases} - - # get all databases, with all filters - kwargs = { - "name_contains": database_name, - "region_eq": region, - } - - databases = rm.databases.get_many(**kwargs) - assert len(databases) > 0 - assert database_name in {db.name for db in databases} + with raises(ValueError): + rm.databases.get_many(region_eq="us-west-2") diff --git a/tests/unit/service/conftest.py b/tests/unit/service/conftest.py index b23ae3e3f6..0759b4609d 100644 --- a/tests/unit/service/conftest.py +++ b/tests/unit/service/conftest.py @@ -65,13 +65,8 @@ def mock_database(db_name: str) -> Database: return Database( name=db_name, description="mock_db_description", - region="", - data_size_full=0, - data_size_compressed=0, create_time=datetime.now().isoformat(), create_actor="", - _attached_engine_names="-", - _errors="", _service=None, ) @@ -81,13 +76,8 @@ def mock_database_2() -> Database: return Database( name="database2", description="completely different db", - region="", - data_size_full=0, - data_size_compressed=0, create_time=datetime.now().isoformat(), create_actor="", - _attached_engine_names="-", - _errors="", _service=None, ) diff --git a/tests/unit/service/test_database.py b/tests/unit/service/test_database.py index 8690fa89f1..2b476e36af 100644 --- a/tests/unit/service/test_database.py +++ b/tests/unit/service/test_database.py @@ -7,7 +7,7 @@ from firebolt.model.V2.database import Database from firebolt.model.V2.engine import Engine from firebolt.service.manager import ResourceManager -from firebolt.utils.exception import AttachedEngineInUseError, OperationalError +from firebolt.utils.exception import AttachedEngineInUseError def test_database_create( @@ -179,30 +179,3 @@ def test_database_update_with_attached_engine_in_use( with raises(AttachedEngineInUseError): mock_database.update(description="new description") - - -def test_database_catalog_name( - httpx_mock: HTTPXMock, - select_one_query_callback: Callable, - system_engine_no_db_query_url: str, - resource_manager: ResourceManager, -): - httpx_mock.add_callback( - select_one_query_callback, url=system_engine_no_db_query_url - ) - - assert resource_manager.databases.catalog_name == "catalog" - - -def test_database_catalog_name_backup( - httpx_mock: HTTPXMock, - select_one_query_callback: Callable, - system_engine_no_db_query_url: str, - resource_manager: ResourceManager, -): - def failure_callback(*args, **kwargs) -> Response: - raise OperationalError("Catalogs don't exist") - - httpx_mock.add_callback(failure_callback, url=system_engine_no_db_query_url) - - assert resource_manager.databases.catalog_name == "database" diff --git a/tests/unit/service/test_engine.py b/tests/unit/service/test_engine.py index d29b351775..ca20db11b5 100644 --- a/tests/unit/service/test_engine.py +++ b/tests/unit/service/test_engine.py @@ -96,9 +96,6 @@ def test_attach_to_database( system_engine_no_db_query_url: str, select_one_query_callback: Callable, ): - httpx_mock.add_callback( - select_one_query_callback, url=system_engine_no_db_query_url - ) httpx_mock.add_callback(database_get_callback, url=system_engine_no_db_query_url) httpx_mock.add_callback(get_engine_callback, url=system_engine_no_db_query_url) httpx_mock.add_callback( From 81d1c8ad269218e512d55a577458aa053af7eafc Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Tue, 2 Jul 2024 16:07:43 +0300 Subject: [PATCH 5/5] remove unused fixture --- tests/unit/service/test_engine.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/service/test_engine.py b/tests/unit/service/test_engine.py index ca20db11b5..8e16deb3b0 100644 --- a/tests/unit/service/test_engine.py +++ b/tests/unit/service/test_engine.py @@ -94,7 +94,6 @@ def test_attach_to_database( database_get_callback: Callable, attach_engine_to_db_callback: Callable, system_engine_no_db_query_url: str, - select_one_query_callback: Callable, ): httpx_mock.add_callback(database_get_callback, url=system_engine_no_db_query_url) httpx_mock.add_callback(get_engine_callback, url=system_engine_no_db_query_url)