twalthr commented on code in PR #25810:
URL: https://github.com/apache/flink/pull/25810#discussion_r1891354425


##########
flink-python/pyflink/table/table_environment.py:
##########
@@ -657,19 +657,50 @@ def drop_temporary_table(self, table_path: str) -> bool:
         """
         return self._j_tenv.dropTemporaryTable(table_path)
 
+    def drop_table(self, table_path: str) -> bool:
+        """
+        Drops a table registered in the given path.
+
+        Temporary objects can shadow permanent ones. If a temporary object 
exists in a given path,
+        make sure to drop the temporary object first using 
:func:`drop_temporary_view`.

Review Comment:
   ```suggestion
           make sure to drop the temporary object first using 
:func:`drop_temporary_table`.
   ```



##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -649,6 +649,13 @@ public boolean dropTemporaryTable(String path) {
         }
     }
 
+    @Override
+    public boolean dropTable(String path) {
+        UnresolvedIdentifier unresolvedIdentifier = 
getParser().parseIdentifier(path);
+        ObjectIdentifier identifier = 
catalogManager.qualifyIdentifier(unresolvedIdentifier);
+        return catalogManager.dropTable(identifier, true);

Review Comment:
   just to clarify: this is different from SQL right? `DROP TABLE` in SQL will 
use `ifNotExists=false`. If yes we should also document this in the JavaDoc to 
avoid confusion.
   
   ```
   Compared to SQL, this method will not throw an error if the table does not 
exist. Use {@link #dropTable(.., ignoreIfExists)} to change the default 
behavior.
   ```



##########
flink-python/pyflink/table/table_environment.py:
##########
@@ -657,19 +657,50 @@ def drop_temporary_table(self, table_path: str) -> bool:
         """
         return self._j_tenv.dropTemporaryTable(table_path)
 
+    def drop_table(self, table_path: str) -> bool:
+        """
+        Drops a table registered in the given path.
+
+        Temporary objects can shadow permanent ones. If a temporary object 
exists in a given path,
+        make sure to drop the temporary object first using 
:func:`drop_temporary_view`.
+        This method can only drop permanent objects.
+
+        :param table_path: The path of the registered table.
+        :return: True if a table existed in the given path and was removed.
+
+        .. versionadded:: 2.0.0
+        """
+        return self._j_tenv.dropTable(table_path)
+
     def drop_temporary_view(self, view_path: str) -> bool:
         """
         Drops a temporary view registered in the given path.
 
         If a permanent table or view with a given path exists, it will be used
         from now on for any queries that reference this path.
 
+        :param view_path: The path of the registered temporary view.
         :return: True if a view existed in the given path and was removed.
 
         .. versionadded:: 1.10.0
         """
         return self._j_tenv.dropTemporaryView(view_path)
 
+    def drop_view(self, view_path: str) -> bool:
+        """
+        Drops a view registered in the given path.
+
+        Temporary objects can shadow permanent ones. If a temporary object 
exists in a given path,
+        make sure to drop the temporary object first using 
:func:`drop_temporary_table`.

Review Comment:
   ```suggestion
           make sure to drop the temporary object first using 
:func:`drop_temporary_view`.
   ```



##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/TableEnvironment.java:
##########
@@ -1030,6 +1030,17 @@ void createTemporarySystemFunction(
      */
     boolean dropTemporaryTable(String path);
 
+    /**
+     * Drops a table registered in the given path.

Review Comment:
   ```suggestion
        * Drops a table registered in the given path. Since views are 
considered virtual tables, it can also drop a registered view.
   ```
   Is that correct? If yes, this sentence might avoid confusion.



##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -649,6 +649,13 @@ public boolean dropTemporaryTable(String path) {
         }
     }
 
+    @Override
+    public boolean dropTable(String path) {
+        UnresolvedIdentifier unresolvedIdentifier = 
getParser().parseIdentifier(path);
+        ObjectIdentifier identifier = 
catalogManager.qualifyIdentifier(unresolvedIdentifier);
+        return catalogManager.dropTable(identifier, true);

Review Comment:
   Same for views



-- 
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: issues-unsubscr...@flink.apache.org

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

Reply via email to