[GitHub] [superset] villebro commented on a diff in pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto

[GitBox <gi...@apache.org>]

villebro commented on code in PR #21066:
URL: https://github.com/apache/superset/pull/21066#discussion_r953405378


##########
superset/db_engine_specs/presto.py:
##########
@@ -21,27 +21,24 @@
 import time
 from collections import defaultdict, deque
 from contextlib import closing
-from datetime import datetime
 from distutils.version import StrictVersion
 from typing import Any, cast, Dict, List, Optional, Pattern, Tuple, TYPE_CHECKING, Union
-from urllib import parse
 
 import pandas as pd
-import simplejson as json
 from flask import current_app
 from flask_babel import gettext as __, lazy_gettext as _
 from sqlalchemy import Column, literal_column, types
 from sqlalchemy.engine.base import Engine
 from sqlalchemy.engine.reflection import Inspector
 from sqlalchemy.engine.result import Row as ResultRow
-from sqlalchemy.engine.url import URL
 from sqlalchemy.orm import Session
 from sqlalchemy.sql.expression import ColumnClause, Select
 
 from superset import cache_manager, is_feature_enabled
 from superset.common.db_query_status import QueryStatus
 from superset.databases.utils import make_url_safe
-from superset.db_engine_specs.base import BaseEngineSpec, ColumnTypeMapping
+from superset.db_engine_specs.base import ColumnTypeMapping
+from superset.db_engine_specs.presto_base import PrestoBaseEngineSpec

Review Comment:
   I think we could just as well keep `PrestoBaseEngineSpec` in `superset.db_engine_specs.presto`, like we do for `PostgresBaseEngineSpec`, as it kinda keeps the list of modules in `db_engine_specs` shorter. Not strongly opinionated here, but let's at least keep it consistent; if we do decide to break out the bases, then I'd prefer to do it in a follow-up refactor PR and doing the same for other specs, too.



##########
superset/db_engine_specs/trino.py:
##########
@@ -89,32 +113,6 @@ def get_url_for_impersonation(
     def get_allow_cost_estimate(cls, extra: Dict[str, Any]) -> bool:
         return True
 
-    @classmethod
-    def get_table_names(
-        cls,
-        database: Database,
-        inspector: Inspector,
-        schema: Optional[str],
-    ) -> List[str]:
-        return BaseEngineSpec.get_table_names(
-            database=database,
-            inspector=inspector,
-            schema=schema,
-        )
-
-    @classmethod
-    def get_view_names(
-        cls,
-        database: Database,
-        inspector: Inspector,
-        schema: Optional[str],
-    ) -> List[str]:
-        return BaseEngineSpec.get_view_names(
-            database=database,
-            inspector=inspector,
-            schema=schema,
-        )
-

Review Comment:
   We can also remove `has_implicit_cancel`, as it's no longer needed (it returns the same as `BaseEngineSpec` which isn't overridden in `PrestoBaseEngineSpec`)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org