szehon-ho commented on code in PR #9852:
URL: https://github.com/apache/iceberg/pull/9852#discussion_r1512046593
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
Review Comment:
This is strange, the method is only used by listViews, what is the point of
parameterizing? (Unless we are going to re-use some of the components, see
following comments)
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##########
@@ -181,4 +264,220 @@ default Table newHmsTable(String hmsTableOwner) {
return newTable;
}
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ default void commitWithLocking(
Review Comment:
In general, my comments in this method are that, we should keep the existing
logic as much as possible, as it is quite delicate and we don't want to
re-introduce bugs that have been fixed over the many releases.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
+ Namespace namespace, TableType tableType, String tableTypeProp)
+ throws TException, InterruptedException {
+ String database = namespace.level(0);
+ List<String> tableNames = clients.run(client -> client.getTables(database,
"*", tableType));
+
+ // Retrieving the Table objects from HMS in batches to avoid OOM
+ List<TableIdentifier> filteredTableIdentifiers = Lists.newArrayList();
Review Comment:
This may be a good idea, but I feel it's a bit early now, we dont even do
this for regular tables.
Can't we do this in a subsequent pr for both tables and views?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
+ Namespace namespace, TableType tableType, String tableTypeProp)
+ throws TException, InterruptedException {
+ String database = namespace.level(0);
+ List<String> tableNames = clients.run(client -> client.getTables(database,
"*", tableType));
+
+ // Retrieving the Table objects from HMS in batches to avoid OOM
+ List<TableIdentifier> filteredTableIdentifiers = Lists.newArrayList();
+ Iterable<List<String>> tableNameSets = Iterables.partition(tableNames,
100);
+
+ for (List<String> tableNameSet : tableNameSets) {
+ filteredTableIdentifiers.addAll(filterIcebergTables(tableNameSet,
namespace, tableTypeProp));
+ }
+
+ return filteredTableIdentifiers;
+ }
+
+ private List<TableIdentifier> filterIcebergTables(
+ List<String> tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+ List<Table> tableObjects =
+ clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+ return tableObjects.stream()
+ .filter(
+ table ->
+ table.getParameters() != null
+ && tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+ .map(table -> TableIdentifier.of(namespace, table.getTableName()))
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ @SuppressWarnings("FormatStringAnnotation")
+ public void renameView(TableIdentifier from, TableIdentifier originalTo) {
Review Comment:
Could we have a common method for renameTable and renameView in this class?
I feel a lot of code is exactly same, and we may reduce errors that way.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
+ Namespace namespace, TableType tableType, String tableTypeProp)
+ throws TException, InterruptedException {
+ String database = namespace.level(0);
+ List<String> tableNames = clients.run(client -> client.getTables(database,
"*", tableType));
+
+ // Retrieving the Table objects from HMS in batches to avoid OOM
+ List<TableIdentifier> filteredTableIdentifiers = Lists.newArrayList();
+ Iterable<List<String>> tableNameSets = Iterables.partition(tableNames,
100);
+
+ for (List<String> tableNameSet : tableNameSets) {
+ filteredTableIdentifiers.addAll(filterIcebergTables(tableNameSet,
namespace, tableTypeProp));
+ }
+
+ return filteredTableIdentifiers;
+ }
+
+ private List<TableIdentifier> filterIcebergTables(
Review Comment:
Would be nice to make the listTables() method call this?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
+ Namespace namespace, TableType tableType, String tableTypeProp)
+ throws TException, InterruptedException {
+ String database = namespace.level(0);
+ List<String> tableNames = clients.run(client -> client.getTables(database,
"*", tableType));
+
+ // Retrieving the Table objects from HMS in batches to avoid OOM
+ List<TableIdentifier> filteredTableIdentifiers = Lists.newArrayList();
+ Iterable<List<String>> tableNameSets = Iterables.partition(tableNames,
100);
+
+ for (List<String> tableNameSet : tableNameSets) {
+ filteredTableIdentifiers.addAll(filterIcebergTables(tableNameSet,
namespace, tableTypeProp));
+ }
+
+ return filteredTableIdentifiers;
+ }
+
+ private List<TableIdentifier> filterIcebergTables(
+ List<String> tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+ List<Table> tableObjects =
+ clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+ return tableObjects.stream()
+ .filter(
+ table ->
+ table.getParameters() != null
+ && tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+ .map(table -> TableIdentifier.of(namespace, table.getTableName()))
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ @SuppressWarnings("FormatStringAnnotation")
+ public void renameView(TableIdentifier from, TableIdentifier originalTo) {
+
+ if (!isValidIdentifier(from)) {
+ throw new NoSuchViewException("Invalid identifier: %s", from);
+ }
+
+ if (!namespaceExists(originalTo.namespace())) {
Review Comment:
Do we need this? We dont do it in renameTables.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -269,6 +284,160 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ if (!isValidIdentifier(identifier)) {
+ return false;
+ }
+
+ try {
+ String database = identifier.namespace().level(0);
+ String viewName = identifier.name();
+ Table table = clients.run(client -> client.getTable(database, viewName));
+ HiveViewOperations.validateTableIsIcebergView(
+ table, CatalogUtil.fullTableName(name, identifier));
+ clients.run(
+ client -> {
+ client.dropTable(database, viewName);
+ return null;
+ });
+ LOG.info("Dropped View: {}", identifier);
+ return true;
+
+ } catch (NoSuchViewException | NoSuchObjectException e) {
+ LOG.info("Skipping drop, View does not exist: {}", identifier, e);
+ return false;
+ } catch (TException e) {
+ throw new RuntimeException("Failed to drop " + identifier, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to dropView", e);
+ }
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ Preconditions.checkArgument(
+ isValidateNamespace(namespace), "Missing database in namespace: %s",
namespace);
+
+ try {
+ return listTablesByType(
+ namespace, TableType.VIRTUAL_VIEW,
BaseMetastoreTableOperations.ICEBERG_VIEW_TYPE_VALUE);
+ } catch (UnknownDBException e) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+
+ } catch (TException e) {
+ throw new RuntimeException("Failed to list all views under namespace " +
namespace, e);
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted in call to listViews", e);
+ }
+ }
+
+ private List<TableIdentifier> listTablesByType(
+ Namespace namespace, TableType tableType, String tableTypeProp)
+ throws TException, InterruptedException {
+ String database = namespace.level(0);
+ List<String> tableNames = clients.run(client -> client.getTables(database,
"*", tableType));
+
+ // Retrieving the Table objects from HMS in batches to avoid OOM
+ List<TableIdentifier> filteredTableIdentifiers = Lists.newArrayList();
+ Iterable<List<String>> tableNameSets = Iterables.partition(tableNames,
100);
+
+ for (List<String> tableNameSet : tableNameSets) {
+ filteredTableIdentifiers.addAll(filterIcebergTables(tableNameSet,
namespace, tableTypeProp));
+ }
+
+ return filteredTableIdentifiers;
+ }
+
+ private List<TableIdentifier> filterIcebergTables(
+ List<String> tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+ List<Table> tableObjects =
+ clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+ return tableObjects.stream()
+ .filter(
+ table ->
+ table.getParameters() != null
+ && tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+ .map(table -> TableIdentifier.of(namespace, table.getTableName()))
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ @SuppressWarnings("FormatStringAnnotation")
+ public void renameView(TableIdentifier from, TableIdentifier originalTo) {
+
+ if (!isValidIdentifier(from)) {
+ throw new NoSuchViewException("Invalid identifier: %s", from);
+ }
+
+ if (!namespaceExists(originalTo.namespace())) {
+ throw new NoSuchNamespaceException(
+ "Cannot rename %s to %s. Namespace does not exist: %s",
+ from, originalTo, originalTo.namespace());
+ }
+
+ TableIdentifier to = removeCatalogName(originalTo);
+ Preconditions.checkArgument(isValidIdentifier(to), "Invalid identifier:
%s", to);
+
+ String toDatabase = to.namespace().level(0);
+ String fromDatabase = from.namespace().level(0);
+ String fromName = from.name();
+
+ try {
+ Table fromView = clients.run(client -> client.getTable(fromDatabase,
fromName));
+ HiveViewOperations.validateTableIsIcebergView(
+ fromView, CatalogUtil.fullTableName(name, from));
+
+ validateToTableForRename(from, to);
+
+ fromView.setDbName(toDatabase);
+ fromView.setTableName(to.name());
+
+ clients.run(
+ client -> {
+ MetastoreUtil.alterTable(client, fromDatabase, fromName, fromView);
+ return null;
+ });
+
+ LOG.info("Renamed view from {}, to {}", from, to);
+
+ } catch (NoSuchObjectException | NoSuchViewException e) {
Review Comment:
We need to catch InvalidObjectException, like the case of tables.
--
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...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
