IcebergCatalog- updateNamespaceProperties on single commit by rambleraptor · Pull Request #4013 · apache/polaris

rambleraptor · 2026-03-17T22:54:40Z

First time Polaris contributor here! I've been working mostly on the Iceberg side.

We ran the PyIceberg Catalog Tests against Polaris (apache/iceberg-python#3106) and discovered that Polaris can't handle adding / removing namespace properties at the same time.

This is because Polaris attempts to handle this as two commits (which run into concurrency issues), when the Iceberg spec supports a single commit.

This PR adds support for creating this single commit for Iceberg Catalogs.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

jbonofre · 2026-03-18T04:41:09Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+  public boolean updateProperties(
+      Namespace namespace, Set<String> removals, Map<String, String> updates)
+      throws NoSuchNamespaceException {
+    PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(namespace);


Why not using resolvedEntityView.getResolvedPath(ResolvedPathKey.ofNamespace(namespace)); like in other methods here ?

jbonofre · 2026-03-18T04:43:30Z

...me/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java

-    if (!updates.isEmpty()) {
-      catalog.setProperties(namespace, updates);
-    }
+    if (catalog instanceof IcebergCatalog polarisCatalog) {


Due to that we have a direct dependency with IcebergCatalog.
As CatalogHandlerUtils implements the SupportNamespace, it should not be "linked" to any catalog specific implementation.

I think it should be cleaner to isolate this in a specific method (for clarity).

Yes, having instanceof checks in this case does not look nice from the design POV.

If we have to have custom handling for the internal catalog (which is ok), perhaps it is time to refactor this code to use proper polymorphic catalog objects and allow implementations to decide how to best handle updates.

Federated catalogs will have to delegate to the appropriate API.

nandorKollar · 2026-03-18T10:08:12Z

...me/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java

-      catalog.setProperties(namespace, updates);
-    }
+    if (catalog instanceof IcebergCatalog polarisCatalog) {
+      polarisCatalog.updateProperties(namespace, removals, updates);


nit: I think if both updates and removals are empty, there's no need to call updateProperties no?

adutra · 2026-03-18T14:29:42Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+   * @param updates the map of properties to update
+   * @return true if the properties were updated, false otherwise
+   */
+  public boolean updateProperties(


The returned boolean is never used / checked.

adutra · 2026-03-18T14:32:39Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+    if (!realmConfig.getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) {
+      LOGGER.debug("Validating no overlap with sibling tables or namespaces");
+      validateNoLocationOverlap(
+          NamespaceEntity.of(updatedEntity), resolvedEntities.getRawParentPath());
+    } else {
+      LOGGER.debug("Skipping location overlap validation for namespace '{}'", namespace);
+    }
+    if (!realmConfig.getConfig(
+        BehaviorChangeConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
+      if (updates.containsKey(PolarisEntityConstants.ENTITY_BASE_LOCATION)) {
+        validateNamespaceLocation(NamespaceEntity.of(entity), resolvedEntities);
+      }
+    }


These checks appear twice, can we extract a common utility method?

adutra · 2026-03-18T14:34:19Z

...ice/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java

+
+    catalog.updateProperties(
+        NS,
+        ImmutableSet.of("prop1", "prop2", "missing_prop"),


Nit: let's keep one old prop to assert that it is preserved after the update:

Suggested change

ImmutableSet.of("prop1", "prop2", "missing_prop"),

ImmutableSet.of("prop1", "missing_prop"),

dimas-b · 2026-03-23T18:46:06Z

...ice/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java

@@ -2614,4 +2615,30 @@ public void testPaginatedListNamespaces() {
      }
    }
  }
+
+  @Test
+  public void testUpdateNamespacePropertiesAtomic() {


It might be preferable to promote this test to PolarisRestCatalogIntegrationBase, which acts almost like a TCK for Polaris. WDYT?

dimas-b · 2026-03-23T18:47:06Z

...ice/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java

@@ -2614,4 +2615,30 @@ public void testPaginatedListNamespaces() {
      }
    }
  }
+
+  @Test
+  public void testUpdateNamespacePropertiesAtomic() {


What in this test ensures that the operation is "atomic"? 🤔

dimas-b · 2026-03-23T19:06:48Z

...me/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java

    Set<String> missing = Sets.difference(removals, startProperties.keySet());

-    if (!updates.isEmpty()) {
-      catalog.setProperties(namespace, updates);


Does this mean Polaris propagates namespace property updates to Federated catalogs even if CatalogEntity.isStaticFacade() is true? @dennishuo WDYT?

dimas-b · 2026-03-23T19:09:47Z

...me/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CatalogHandlerUtils.java

-    if (!updates.isEmpty()) {
-      catalog.setProperties(namespace, updates);
-    }
+    if (catalog instanceof IcebergCatalog polarisCatalog) {


Yes, having instanceof checks in this case does not look nice from the design POV.

If we have to have custom handling for the internal catalog (which is ok), perhaps it is time to refactor this code to use proper polymorphic catalog objects and allow implementations to decide how to best handle updates.

Federated catalogs will have to delegate to the appropriate API.

dimas-b · 2026-03-23T19:10:26Z

Thanks for you contribution, @rambleraptor 👍

Iceberg Catalogs should update namespace properties on a single commit

d28fa3a

github-project-automation bot added this to Basic Kanban Board Mar 17, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 17, 2026

Add test

08f5d12

jbonofre reviewed Mar 18, 2026

View reviewed changes

nandorKollar reviewed Mar 18, 2026

View reviewed changes

adutra reviewed Mar 18, 2026

View reviewed changes

dimas-b reviewed Mar 23, 2026

View reviewed changes

dimas-b requested review from HonahX and dennishuo March 23, 2026 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IcebergCatalog- updateNamespaceProperties on single commit#4013

IcebergCatalog- updateNamespaceProperties on single commit#4013
rambleraptor wants to merge 2 commits intoapache:mainfrom
rambleraptor:polaris-commit-fix

rambleraptor commented Mar 17, 2026 •

edited

Loading

Uh oh!

jbonofre Mar 18, 2026

Uh oh!

jbonofre Mar 18, 2026

Uh oh!

dimas-b Mar 23, 2026

Uh oh!

nandorKollar Mar 18, 2026

Uh oh!

adutra Mar 18, 2026

Uh oh!

adutra Mar 18, 2026

Uh oh!

adutra Mar 18, 2026

Uh oh!

dimas-b Mar 23, 2026

Uh oh!

dimas-b Mar 23, 2026

Uh oh!

dimas-b Mar 23, 2026

Uh oh!

dimas-b Mar 23, 2026

Uh oh!

dimas-b commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	ImmutableSet.of("prop1", "prop2", "missing_prop"),
	ImmutableSet.of("prop1", "missing_prop"),

Conversation

rambleraptor commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rambleraptor commented Mar 17, 2026 •

edited

Loading