Correctly Propagate Errors from Drop Entities

[travis-bowen]

During drop commands, Apache Polaris would make assumptions based on the entity type as to why the entity could not be dropped which could lead to incorrect user error messaging and handling.

Here I tackle updating error handling for DropTable, DropNamespace, and DropView.

The Catalog API javadocs by Iceberg indicate false should indicate [TableNotExists] (and simila for View) (https://github.com/apache/iceberg/blob/a8ece055ba93adc0c046db29b6b7b5edaf35d4da/api/src/main/java/org/apache/iceberg/catalog/Catalog.java#L292).

For Namespaces it's less clear, but I assume the same holds and we treat it to follow the same logic that false = Not Found in CatalogHandlerUtils.

Before
Any non success in Polaris's IcebergCatalog impl would return false.
Polaris's CatalogHandlerUtils would interpret this false to mean the entity did not exist and would throw the appropriate does not exist error

However, the reason that it returned false, may not truly be that it didn't exist.

After
The IcebergCatalog impl in Polaris uses the return status of the persistence result to determine whether or not the drop failure was due to being not found. If not found, it returns false per the interface definition, which the CatalogHandlerUtil transforms into the appropriate not found exception. Otherwise it throws it's own error corresponding to the cause.

Ran tests locally + S3 Tests with drop

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)

[flyrain]

Thanks @travis-bowen for the fix. LGTM overall. Left a few minor comments.

I'm not a big fan of the BaseResult and its subclasses, I think a better way is to create a set of exceptions which can be threw from the persistence layer(AtomicOperationMetaStoreManager, TransactionalMetaStoreManagerImpl). It's a different topic and fairly big change though. We could handle that later.

[HonahX]

Overall LGTM!
Quick question: Do we need ENTITY_UNDROPPABLE for table/view? I remember this is for service_admin and catalog_admin role which are not droppable.

[HonahX]

Overall LGTM!

[HonahX]

Do we need this for table/view? I remember this is for service_admin and catalog_admin role which are not droppable.

[travis-bowen]

You're right - we don't need it. I lean towards keeping it since we can't be certain of any custom implementations but I'm not tied to keeping it if you feel strongly.