Bound the catalog-role count taken when dropping a catalog

[ayushtkn]

Dropping a catalog needs to answer one question — does it still hold more than one catalog role? — and it asked listFullEntities for that with a page size of 2:

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java

Lines 1160 to 1171 in 3029551

	// get the list of catalog roles, at most 2
	List<PolarisBaseEntity> catalogRoles =
	ms.listFullEntities(
	callCtx,
	catalogId,
	catalogId,
	PolarisEntityType.CATALOG_ROLE,
	PolarisEntitySubType.ANY_SUBTYPE,
	entity -> true,
	Function.identity(),
	PageToken.fromLimit(2))
	.items();

So the drop issued an unbounded SELECT of all 16 columns — both JSON property blobs included — returning every catalog role in the catalog, to answer a yes/no question.

The method's own javadoc already points the other way:

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java

Lines 301 to 303 in 3029551

	/**
	* Load full entities matching the given criteria with pagination and transformation. If only the
	* entity name/id/type is required, use {@link #listEntities} instead.

Only the identity of at most two roles is needed here, so this switches to listEntities, which is projected (ENTITY_LOOKUP_COLUMNS) and does apply the limit. The single remaining role is then looked up by id in order to drop it, which also covers it having been dropped concurrently.

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)

[Copilot]

🟢 Approval recommended

The remaining request is a minor regression-test nit, with no approval-blocking issues.

Pull request overview

Bounds catalog-role lookups during catalog deletion and avoids loading unnecessary full records.

Changes:

• Uses projected, limited role listing.
• Reloads the remaining role before deletion.
• Adds regression coverage for the bounded lookup.

File summaries

File	Description
polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisTreeMapAtomicOperationMetaStoreManagerTest.java	Verifies the role listing uses a page size of two.
polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java	Implements bounded role listing and lookup.

Review details

Suppressed comments (1)

polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java:1188

• Please add a regression test for the new concurrent-drop path. The added test only captures the page size and never exercises lookupEntity returning null; a future change could reintroduce a failure when the single role disappears between listing and lookup while this test still passes. Stub one role record, make the role lookup return null, and assert that dropping the catalog completes successfully.

        // null means it was dropped concurrently, which leaves nothing to do
        if (catalogRoleToDrop != null) {
          this.dropEntity(callCtx, ms, catalogRoleToDrop);

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vigneshio]

LGTM, Thanks @ayushtkn

[flyingImer]

LGTM. The bounded catalog-role lookup makes sense.