[GitHub] [superset] zhaoyongjie commented on a change in pull request #16624: fix(dataset): create ES-View dataset raise exception #16623

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

zhaoyongjie commented on a change in pull request #16624:
URL: https://github.com/apache/superset/pull/16624#discussion_r704949580



##########
File path: superset/connectors/sqla/utils.py
##########
@@ -43,7 +43,7 @@ def get_physical_table_metadata(
     # ensure empty schema
     _schema_name = schema_name if schema_name else None
     # Table does not exist or is not visible to a connection.
-    if not database.has_table_by_name(table_name, schema=_schema_name):
+    if not database.has_table_or_view_by_name(table_name, schema=_schema_name):

Review comment:
       Could we decouple `has_table_by_name` and `has_view_by_name`?
   
   ```python
       if any([
           database.has_table_by_name(table_name, schema=_schema_name), 
           database.has_view_by_name(table_name, schema=_schema_name), 
       ]):
           ....
   ```

##########
File path: superset/db_engine_specs/base.py
##########
@@ -1343,6 +1343,21 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
 
         return False
 
+    @classmethod
+    def _has_view(
+        cls,
+        conn: Connection,
+        dialect: Dialect,
+        view_name: str,
+        schema: Optional[str] = None,
+    ):
+        view_names = dialect.get_view_names(connection=conn, schema=schema)

Review comment:
       should catch `NotImplementedError` here

##########
File path: superset/models/core.py
##########
@@ -721,6 +721,20 @@ def has_table_by_name(self, table_name: str, schema: Optional[str] = None) -> bo
         engine = self.get_sqla_engine()
         return engine.has_table(table_name, schema)
 
+    def has_view_by_name(self, view_name: str, schema: Optional[str] = None) -> bool:
+        engine = self.get_sqla_engine()
+        return db_engine_specs.BaseEngineSpec.has_view(
+            engine=engine, view_name=view_name, schema=schema
+        )
+
+    def has_table_or_view_by_name(

Review comment:
       same before, decouple `has_table_by_name` and `has_view_by_name`

##########
File path: superset/db_engine_specs/base.py
##########
@@ -1343,6 +1343,21 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
 
         return False
 
+    @classmethod
+    def _has_view(
+        cls,
+        conn: Connection,
+        dialect: Dialect,
+        view_name: str,
+        schema: Optional[str] = None,
+    ):
+        view_names = dialect.get_view_names(connection=conn, schema=schema)
+        return view_name in view_names
+
+    @classmethod
+    def has_view(cls, engine: Engine, view_name: str, schema: Optional[str] = None):

Review comment:
       Could we move `has_view` to `superset.models.core` in order to follow `has_table` pattern.




-- 
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