[GitHub] [superset] john-bodley commented on a diff in pull request #21794: fix(trino): Ensure get_table_names only returns physical tables

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

john-bodley commented on code in PR #21794:
URL: https://github.com/apache/superset/pull/21794#discussion_r994136766


##########
tests/integration_tests/db_engine_specs/presto_tests.py:
##########
@@ -33,42 +33,29 @@ class TestPrestoDbEngineSpec(TestDbEngineSpec):
     def test_get_datatype_presto(self):
         self.assertEqual("STRING", PrestoEngineSpec.get_datatype("string"))
 
-    def test_presto_get_view_names_return_empty_list(

Review Comment:
   I'm not sure what the purpose of this function was, i.e., the logic can be inferred.



##########
superset/db_engine_specs/presto.py:
##########
@@ -392,28 +392,49 @@ def update_impersonation_config(
 
     @classmethod
     def get_table_names(
-        cls, database: Database, inspector: Inspector, schema: Optional[str]
+        cls,
+        database: Database,
+        inspector: Inspector,
+        schema: Optional[str],
     ) -> List[str]:
-        tables = super().get_table_names(database, inspector, schema)
-        if not is_feature_enabled("PRESTO_SPLIT_VIEWS_FROM_TABLES"):

Review Comment:
   I'm not quite sure why this undocumented feature flag existed. Why wouldn't one want to split views from tables as is the case with the other engine specifications.



##########
tests/integration_tests/db_engine_specs/presto_tests.py:
##########
@@ -663,50 +650,17 @@ def test_get_sqla_column_type(self):
         sqla_type = PrestoEngineSpec.get_sqla_column_type(None)
         assert sqla_type is None
 
-    @mock.patch(
-        "superset.utils.feature_flag_manager.FeatureFlagManager.is_feature_enabled"
-    )
     @mock.patch("superset.db_engine_specs.base.BaseEngineSpec.get_table_names")
     @mock.patch("superset.db_engine_specs.presto.PrestoEngineSpec.get_view_names")
-    def test_get_table_names_no_split_views_from_tables(
-        self, mock_get_view_names, mock_get_table_names, mock_is_feature_enabled
-    ):
-        mock_get_view_names.return_value = ["view1", "view2"]
-        table_names = ["table1", "table2", "view1", "view2"]
-        mock_get_table_names.return_value = table_names
-        mock_is_feature_enabled.return_value = False
-        tables = PrestoEngineSpec.get_table_names(mock.Mock(), mock.Mock(), None)
-        assert tables == table_names
-
-    @mock.patch(
-        "superset.utils.feature_flag_manager.FeatureFlagManager.is_feature_enabled"
-    )
-    @mock.patch("superset.db_engine_specs.base.BaseEngineSpec.get_table_names")
-    @mock.patch("superset.db_engine_specs.presto.PrestoEngineSpec.get_view_names")
-    def test_get_table_names_split_views_from_tables(
-        self, mock_get_view_names, mock_get_table_names, mock_is_feature_enabled
+    def test_get_table_names(
+        self,
+        mock_get_view_names,
+        mock_get_table_names,
     ):
         mock_get_view_names.return_value = ["view1", "view2"]
-        table_names = ["table1", "table2", "view1", "view2"]
-        mock_get_table_names.return_value = table_names
-        mock_is_feature_enabled.return_value = True
-        tables = PrestoEngineSpec.get_table_names(mock.Mock(), mock.Mock(), None)
-        assert sorted(tables) == sorted(table_names)

Review Comment:
   This is where I'm puzzled. The result—`["table1", "table2", "view1", "view2"]`—is the same for `test_get_table_names_no_split_views_from_tables` and `test_get_table_names_split_views_from_tables` even though feature is disabled and enabled respectively. When the feature is enabled the result should be `["table1", "table2"]`, i.e., sans views.



##########
superset/db_engine_specs/presto.py:
##########
@@ -392,28 +392,49 @@ def update_impersonation_config(
 
     @classmethod
     def get_table_names(
-        cls, database: Database, inspector: Inspector, schema: Optional[str]
+        cls,
+        database: Database,
+        inspector: Inspector,
+        schema: Optional[str],
     ) -> List[str]:
-        tables = super().get_table_names(database, inspector, schema)
-        if not is_feature_enabled("PRESTO_SPLIT_VIEWS_FROM_TABLES"):
-            return tables
+        """
+        Get all the physical table names from the specified schema.
+
+        Presto's perspective is the table concept represents both physical and non-
+        physical tables (views).
+
+        Note if the schema is omitted, per PyHive's implementation, it defaults to
+        returning all the tables within the database.
+
+        :param database: The database to inspect
+        :param inspector: The SQLAlchemy inspector
+        :param schema: The schema to inspect
+        :returns: The physical table names
+        """
 
-        views = set(cls.get_view_names(database, inspector, schema))
-        actual_tables = set(tables) - views
-        return list(actual_tables)
+        return list(

Review Comment:
   Granted this logic is the same for both Presto and Trino and thus could go in the base class, however there's a subtle difference in how these engines handle an undefined schema which is documented.



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