Airblader commented on a change in pull request #17155:
URL: https://github.com/apache/flink/pull/17155#discussion_r703200043
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOptionsOperation.java
##########
@@ -22,15 +22,32 @@
import org.apache.flink.table.catalog.ObjectIdentifier;
import org.apache.flink.table.operations.OperationUtils;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.Map;
import java.util.stream.Collectors;
-/** Operation to describe a ALTER TABLE .. SET .. statement. */
+/** Operation to describe a ALTER TABLE IF EXISTS .. SET .. statement. */
public class AlterTableOptionsOperation extends AlterTableOperation {
private final CatalogTable catalogTable;
+ private final Map<String, String> tableOptions;
Review comment:
I'm still not really happy with this; now we added redundant information
for the sake of the summary string, but this information isn't actually used by
`CatalogManager` to alter the table.
In principle I think this class shouldn't hold a `CatalogTable` at all, but
rather only an identifier (which it already does from its superclass) and the
new options to be added. `TableEnvironmentImpl#executeInternal` then just looks
up the table when needed (as it already does for some other operations).
Happy to hear someone else's thoughts on this as well.
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java
##########
@@ -759,14 +759,44 @@ private void dropTemporaryTableInternal(
*/
public void alterTable(
CatalogBaseTable table, ObjectIdentifier objectIdentifier, boolean
ignoreIfNotExists) {
- execute(
- (catalog, path) -> {
- final CatalogBaseTable resolvedTable =
resolveCatalogBaseTable(table);
- catalog.alterTable(path, resolvedTable, ignoreIfNotExists);
- },
- objectIdentifier,
- ignoreIfNotExists,
- "AlterTable");
+ alterTableInternal(table, objectIdentifier, ignoreIfNotExists, true);
+ }
+
+ /**
+ * Alters a view in a given fully qualified path.
+ *
+ * @param view The view to put in the given path
+ * @param objectIdentifier The fully qualified path where to alter the
view.
+ * @param ignoreIfNotExists If false exception will be thrown if the view
or database or catalog
+ * to be altered does not exist.
+ */
+ public void alterView(
+ CatalogBaseTable view, ObjectIdentifier objectIdentifier, boolean
ignoreIfNotExists) {
+ alterTableInternal(view, objectIdentifier, ignoreIfNotExists, false);
+ }
+
+ public void alterTableInternal(
+ CatalogBaseTable table,
+ ObjectIdentifier objectIdentifier,
+ boolean ignoreIfNotExists,
+ boolean isAlterTable) {
Review comment:
Maybe it would be a bit nicer to pass a `TableKind kind` rather than a
boolean? We could make the same change for `dropTableInternal`.
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOptionsOperation.java
##########
@@ -22,15 +22,32 @@
import org.apache.flink.table.catalog.ObjectIdentifier;
import org.apache.flink.table.operations.OperationUtils;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.Map;
import java.util.stream.Collectors;
-/** Operation to describe a ALTER TABLE .. SET .. statement. */
+/** Operation to describe a ALTER TABLE IF EXISTS .. SET .. statement. */
public class AlterTableOptionsOperation extends AlterTableOperation {
private final CatalogTable catalogTable;
+ private final Map<String, String> tableOptions;
Review comment:
nit: maybe call this something like `addedOptions` to make it clear that
it only holds options which will be added to already existing options?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
