Copilot commented on code in PR #4730:
URL: https://github.com/apache/polaris/pull/4730#discussion_r3404193221
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1035,8 +1035,44 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP,
catalogEntity);
+ ViewOperations ops = newViewOps(identifier);
+ ViewMetadata lastMetadata;
+ if (purge && ops.current() != null) {
+ lastMetadata = ops.current();
+ } else {
+ lastMetadata = null;
+ }
+
+ PolarisResolvedPathWrapper resolvedViewEntities =
+ resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofTableLike(identifier),
PolarisEntitySubType.ICEBERG_VIEW);
+ PolarisResolvedPathWrapper storageHierarchy =
+ resolvedViewEntities != null
+ ? resolvedViewEntities
+ : resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofNamespace(identifier.namespace()));
+ Optional<PolarisEntity> storageInfoEntity =
+ FileIOUtil.findStorageInfoFromHierarchy(storageHierarchy);
+
+ Map<String, String> storageProperties =
+ storageInfoEntity
+ .map(PolarisEntity::getInternalPropertiesAsMap)
+ .map(
+ properties -> {
+ if (lastMetadata == null || lastMetadata.location() == null)
{
+ return Map.<String, String>of();
+ }
+ Map<String, String> clone = new HashMap<>();
+ clone.putAll(lastMetadata.properties());
+ clone.put(CatalogProperties.FILE_IO_IMPL, ioImplClassName);
+ clone.putAll(properties);
+ clone.put(PolarisTaskConstants.STORAGE_LOCATION,
lastMetadata.location());
+ return clone;
+ })
+ .orElse(Map.of());
Review Comment:
When `storageInfoEntity` is empty, this returns `Map.of()` even if
`lastMetadata` is present, which drops the view metadata-derived properties
(including `PolarisTaskConstants.STORAGE_LOCATION`). That can prevent the
cleanup handler from locating view metadata files in cases where storage info
isn’t resolvable from the hierarchy. Consider always building a base map from
`lastMetadata` (when present) and then optionally merging storage entity
properties if available, rather than gating everything behind
`storageInfoEntity`.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1035,8 +1035,44 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP,
catalogEntity);
+ ViewOperations ops = newViewOps(identifier);
+ ViewMetadata lastMetadata;
+ if (purge && ops.current() != null) {
+ lastMetadata = ops.current();
+ } else {
+ lastMetadata = null;
+ }
+
+ PolarisResolvedPathWrapper resolvedViewEntities =
+ resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofTableLike(identifier),
PolarisEntitySubType.ICEBERG_VIEW);
+ PolarisResolvedPathWrapper storageHierarchy =
+ resolvedViewEntities != null
+ ? resolvedViewEntities
+ : resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofNamespace(identifier.namespace()));
+ Optional<PolarisEntity> storageInfoEntity =
+ FileIOUtil.findStorageInfoFromHierarchy(storageHierarchy);
Review Comment:
The storage hierarchy resolution and storage-info lookup run regardless of
`purge`. If `purge` is false, this is unnecessary work on every `dropView`.
Consider wrapping the metadata/storageProperties construction behind `if
(purge)` and passing `Map.of()` otherwise.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1035,8 +1035,44 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP,
catalogEntity);
+ ViewOperations ops = newViewOps(identifier);
Review Comment:
The storage hierarchy resolution and storage-info lookup run regardless of
`purge`. If `purge` is false, this is unnecessary work on every `dropView`.
Consider wrapping the metadata/storageProperties construction behind `if
(purge)` and passing `Map.of()` otherwise.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1035,8 +1035,44 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP,
catalogEntity);
+ ViewOperations ops = newViewOps(identifier);
+ ViewMetadata lastMetadata;
+ if (purge && ops.current() != null) {
+ lastMetadata = ops.current();
+ } else {
+ lastMetadata = null;
+ }
+
+ PolarisResolvedPathWrapper resolvedViewEntities =
+ resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofTableLike(identifier),
PolarisEntitySubType.ICEBERG_VIEW);
+ PolarisResolvedPathWrapper storageHierarchy =
+ resolvedViewEntities != null
+ ? resolvedViewEntities
+ : resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofNamespace(identifier.namespace()));
+ Optional<PolarisEntity> storageInfoEntity =
+ FileIOUtil.findStorageInfoFromHierarchy(storageHierarchy);
+
+ Map<String, String> storageProperties =
+ storageInfoEntity
+ .map(PolarisEntity::getInternalPropertiesAsMap)
+ .map(
+ properties -> {
+ if (lastMetadata == null || lastMetadata.location() == null)
{
+ return Map.<String, String>of();
+ }
+ Map<String, String> clone = new HashMap<>();
+ clone.putAll(lastMetadata.properties());
+ clone.put(CatalogProperties.FILE_IO_IMPL, ioImplClassName);
+ clone.putAll(properties);
+ clone.put(PolarisTaskConstants.STORAGE_LOCATION,
lastMetadata.location());
+ return clone;
+ })
+ .orElse(Map.of());
+
DropEntityResult dropEntityResult =
- dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(),
purge);
+ dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier,
storageProperties, purge);
Review Comment:
The storage hierarchy resolution and storage-info lookup run regardless of
`purge`. If `purge` is false, this is unnecessary work on every `dropView`.
Consider wrapping the metadata/storageProperties construction behind `if
(purge)` and passing `Map.of()` otherwise.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1035,8 +1035,44 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP,
catalogEntity);
+ ViewOperations ops = newViewOps(identifier);
+ ViewMetadata lastMetadata;
+ if (purge && ops.current() != null) {
+ lastMetadata = ops.current();
+ } else {
+ lastMetadata = null;
+ }
Review Comment:
`ops.current()` is invoked twice. If `current()` touches remote state or
does non-trivial work, this duplicates effort and makes behavior harder to
reason about. Consider calling `ops.current()` once (e.g., store in a local
variable) and then applying the `purge`/null logic.
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -1901,6 +1904,49 @@ public void testDropTableWithPurge() {
.isInstanceOf(InMemoryFileIO.class);
}
+ @Test
+ public void testDropViewWithPurge() {
+ if (this.requiresNamespaceCreate()) {
+ catalog.createNamespace(NS);
+ }
+
+ catalog
+ .buildView(TABLE)
+ .withSchema(SCHEMA)
+ .withDefaultNamespace(NS)
+ .withQuery("spark", "SELECT * FROM ns.tbl")
+ .create();
+
+ Assertions.assertThatPredicate(catalog::viewExists)
+ .as("View should exist after create")
+ .accepts(TABLE);
+
+ boolean dropped = catalog.dropView(TABLE);
+ Assertions.assertThat(dropped).as("Should drop a view that does
exist").isTrue();
+ Assertions.assertThatPredicate(catalog::viewExists)
+ .as("View should not exist after drop")
+ .rejects(TABLE);
+
+ List<PolarisBaseEntity> tasks =
+ metaStoreManager
+ .loadTasks(polarisContext, "testExecutor", PageToken.fromLimit(1))
+ .getEntities();
+ Assertions.assertThat(tasks)
+ .as("A cleanup task should be created when dropping a view with purge
enabled")
+ .hasSize(1);
Review Comment:
This assumes there are no pre-existing tasks and that the newest/desired
task is returned within `fromLimit(1)`. If the underlying task store isn’t
guaranteed empty per test or ordering isn’t guaranteed, this can become flaky.
A more robust assertion is to capture task count before the drop and assert it
increments after, or fetch more than one task and assert that at least one task
contains `PolarisTaskConstants.STORAGE_LOCATION` (or otherwise matches the
expected cleanup task characteristics).
--
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]
