Skip to content

Conversation

@hantangwangd
Copy link
Contributor

This PR fix the failure test RESTCompatibilityKitCatalogTests.listNamespacesWithEmptyNamespace() running based on RESTSessionCatalog.

In the original implementation, for empty namespaces, RESTSessionCatalog.namespaceExists() would directly throw an NoSuchNamespaceException rather than returning false. This will lead to the failure of the test.

@github-actions github-actions bot added the core label Jan 14, 2025
@hantangwangd hantangwangd marked this pull request as draft January 14, 2025 06:49
@hantangwangd hantangwangd marked this pull request as ready for review January 14, 2025 07:42
@hantangwangd
Copy link
Contributor Author

Hi @nastra , can you please take a look at this fix when available? Thanks a lot!

checkNamespaceIsValid(namespace);

try {
checkNamespaceIsValid(namespace);
Copy link
Contributor

@nastra nastra Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please do the same for viewExists and tableExists?

--- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -438,9 +438,9 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
   @Override
   public boolean tableExists(SessionContext context, TableIdentifier identifier) {
     Endpoint.check(endpoints, Endpoint.V1_TABLE_EXISTS);
-    checkIdentifierIsValid(identifier);

     try {
+      checkIdentifierIsValid(identifier);
       client.head(paths.table(identifier), headers(context), ErrorHandlers.tableErrorHandler());
       return true;
     } catch (NoSuchTableException e) {
@@ -659,9 +659,9 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
   @Override
   public boolean namespaceExists(SessionContext context, Namespace namespace) {
     Endpoint.check(endpoints, Endpoint.V1_NAMESPACE_EXISTS);
-    checkNamespaceIsValid(namespace);

     try {
+      checkNamespaceIsValid(namespace);
       client.head(
           paths.namespace(namespace), headers(context), ErrorHandlers.namespaceErrorHandler());
       return true;
@@ -1233,9 +1233,9 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
   @Override
   public boolean viewExists(SessionContext context, TableIdentifier identifier) {
     Endpoint.check(endpoints, Endpoint.V1_VIEW_EXISTS);
-    checkViewIdentifierIsValid(identifier);

     try {
+      checkViewIdentifierIsValid(identifier);
       client.head(paths.view(identifier), headers(context), ErrorHandlers.viewErrorHandler());
       return true;
     } catch (NoSuchViewException e) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@hantangwangd hantangwangd force-pushed the fix_test_failure_about_empty_namespace branch from b2b4060 to ba91203 Compare January 14, 2025 08:07
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing this @hantangwangd, LGTM

@hantangwangd
Copy link
Contributor Author

@nastra My pleasure!

@nastra nastra merged commit c98d0d0 into apache:main Jan 14, 2025
46 checks passed
@hantangwangd hantangwangd deleted the fix_test_failure_about_empty_namespace branch January 14, 2025 09:49
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late +1 to this PR, noticed the issue last night. Thanks for the fix @hantangwangd!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants