Fix nosql namespace / table name clash by Subham-KRLX · Pull Request #4148 · apache/polaris

Subham-KRLX · 2026-04-08T17:20:46Z

Resolves test-crashing IllegalArgumentException errors in NoSQL persistence when creating a table that clashes with an existing namespace name.

Changes:

Added a proactive persistence-neutral clash check to IcebergCatalogHandler allowing for user-friendly AlreadyExistsException messages.
Added a robustness fix to MutationAttempt.java to prevent the NoSQL metastore from crashing during mapToObj processing.
Added reproduction test testNamespaceTableClash.

Fixes #4108

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Closes Obscure error message on table name clash with a namespace in NoSQL Persistence #4108
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic

@SuppressWarnings

- Added missing closing brace in fallbackToFullLoadTable method - Fixed authorizeLoadTable call to use boolean instead of EnumSet - Added @SuppressWarnings for Error Prone FormatStringAnnotation - Updated error message formatting to be compile-safe

Subham-KRLX · 2026-04-08T18:06:04Z

@dimas-b Fixed the NoSQL table/namespace name clash by adding a pre-check that returns a clear AlreadyExistsException instead of the obscure metastore crash plus a small NoSQL robustness fix and a regression test for the clash case.

dimas-b

Thanks for the quick fix, @Subham-KRLX !

dimas-b · 2026-04-08T21:21:20Z

+        var existingTypeId = existingRef.type();
+        var expectedTypeId = objTypeForPolarisType(entityType, entity.getSubType()).id();
+
+        if (existingTypeId.equals(expectedTypeId)) {


Nice find! I think this fix is sufficient. Changes in IcebergCatalogHandler are probably not necessary. WDYT?

Could you add a test for NS / Table name collision to PolarisRestCatalogIntegrationBase?

Yea - this should be the right place.

This case is NoSQL-specific The shared test would fail on file-backed catalogs so I am opting in only from the NoSQL integration test that reproduces the issue.

Fair enough. Let's keep the fix narrow and specific to the issue 👍

adutra · 2026-04-09T14:46:24Z

    return externalCatalogBaseLocation;
  }

+  protected boolean testNamespaceTableNameCollision() {


Why wouldn't we test this for all subclasses?

This case only reproduces on the NoSQL backend. Running it across all subclasses fails for file-backed catalogs, so I kept the shared test and enable it only where the bug is actually reproducible.

Sure, but the test is valid regardless of the backend. (We could also add tests for other clashes: view vs ns, view vs table – this one maybe exists already).

I tested this across all subclasses and file-backed currently behaves differently, so I kept this PR scoped to the NoSQL regression in #4108. I’ll follow up with a separate PR to unify clash behavior across backends and add the extra clash tests.

How about we remove this test from current PR completely and re-add it in a new PR with persistence-neutral fixes?

Tests added to TestNoSqlMetaStoreManager appear to be sufficient for the current production code change and the use case from #4108.

Extending to other backends makes sense though, yet it might be best done in a dedicated PR 🤔

I have just updated the PR to completely remove the shared integration test keeping the scope narrow and strictly focused on the NoSQL regression fix as the tests in TestNoSqlMetaStoreManager are sufficient for this specific issue. I will follow up with a dedicated PR to handle the persistence neutral fixes properly and comprehensively add these collision tests across all backends as suggested.

Thx, @Subham-KRLX ! I opened #4168 for followup.

@Subham-KRLX : Do you feel like working on #4168 ?.. Just checking 😉

dimas-b · 2026-04-09T21:07:09Z

-    authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
-        op, TableIdentifier.of(namespace, request.name()));
+    TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+    authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);


This change looks unnecessary now 🤔

dimas-b · 2026-04-09T21:07:30Z

   */
  public ImmutableLoadCredentialsResponse loadCredentials(
      TableIdentifier tableIdentifier, Optional<String> refreshCredentialsEndpoint) {
-


nit: unnecessary change

dimas-b · 2026-04-09T21:07:49Z

-    authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
-        op, TableIdentifier.of(namespace, request.name()));
+    TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+    authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);


This change looks unnecessary now 🤔

…tests to focus on NoSQL fix

adutra · 2026-04-10T13:50:06Z

 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.catalog.TableCommit;
 import org.apache.iceberg.catalog.TableIdentifier;
+


Still some unnecessary changes in this file.

+1

@Subham-KRLX : please also fix spotless checks

I fixed spotless for this PR and pushed the update.

adutra · 2026-04-10T13:50:27Z

 @QuarkusIntegrationTest
 @TestProfile(value = NoSqlCatalogIT.Profile.class)
 public class NoSqlCatalogIT extends PolarisRestCatalogRustFSIT {
+


I removed the unnecessary change in NoSqlCatalogIT.java and kept only the spotless-required formatting updates in this PR.I removed the unnecessary change in NoSqlCatalogIT.java and kept only the spotless-required formatting updates in this PR.

dimas-b

Thanks, @Subham-KRLX ! Let's give some more time for @adutra to catch up with recent changes, and I hope we can merge on Mon.

github-project-automation Bot added this to Basic Kanban Board Apr 8, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Apr 8, 2026

Subham-KRLX force-pushed the fix/nosql-namespace-table-clash-4108 branch 2 times, most recently from ca3e04a to 0cb1f42 Compare April 8, 2026 17:27

dimas-b reviewed Apr 8, 2026

View reviewed changes

Fix NoSQL namespace/table name clash handling

20f0fb4

Subham-KRLX force-pushed the fix/nosql-namespace-table-clash-4108 branch from 3b5d8b4 to 20f0fb4 Compare April 9, 2026 03:28

adutra reviewed Apr 9, 2026

View reviewed changes

Subham-KRLX added 2 commits April 9, 2026 21:04

Clarify namespace/table collision test scope

0822c7d

Fix spotless formatting for collision test comment

51eedd0

dimas-b reviewed Apr 9, 2026

View reviewed changes

Cleanup PR apache#4148: Revert unnecessary changes and remove shared …

ef58c5a

…tests to focus on NoSQL fix

adutra reviewed Apr 10, 2026

View reviewed changes

dimas-b mentioned this pull request Apr 10, 2026

Add entity name overlap tests #4168

Open

Fix spotless formatting in collision-test touched files

a944a89

dimas-b approved these changes Apr 10, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Apr 10, 2026

dimas-b changed the title ~~Fix/nosql namespace table clash 4108~~ Fix nosql namespace / table name clash Apr 10, 2026

adutra approved these changes Apr 13, 2026

View reviewed changes

dimas-b merged commit 648c8ee into apache:main Apr 13, 2026
20 checks passed

github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Apr 13, 2026

Conversation

Subham-KRLX commented Apr 8, 2026 • edited by dimas-b Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Subham-KRLX commented Apr 8, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Subham-KRLX commented Apr 8, 2026 •

edited by dimas-b

Loading

dimas-b Apr 8, 2026 •

edited

Loading