[GitHub] [iceberg] aajisaka commented on a diff in pull request #6586: AWS: make warehouse path optional for read only catalog use cases

[GitBox <gi...@apache.org>]

aajisaka commented on code in PR #6586:
URL: https://github.com/apache/iceberg/pull/6586#discussion_r1082123777


##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -132,6 +134,28 @@ public void testCreateTableBadName() {
                 TableIdentifier.of(namespace, "table-1"), schema, partitionSpec));
   }
 
+  @Test
+  public void testCreateAndLoadTableWithoutWarehouseLocation() {
+    GlueCatalog glueCatalogWithoutWarehouse = new GlueCatalog();
+    glueCatalogWithoutWarehouse.initialize(
+        catalogName,
+        null,
+        new AwsProperties(),
+        glue,
+        LockManagers.defaultLockManager(),
+        new S3FileIO(clientFactory::s3),
+        ImmutableMap.of());
+    String namespace = createNamespace();
+    String tableName = getRandomName();
+    TableIdentifier identifier = TableIdentifier.of(namespace, tableName);
+    try {
+      glueCatalog.createTable(identifier, schema, partitionSpec, tableLocationProperties);
+      glueCatalog.loadTable(identifier);
+    } catch (RuntimeException e) {
+      fail("Create and load table without warehouse location should succeed");

Review Comment:
   ```suggestion
         throw new IOException("Create and load table without warehouse location should succeed", e);
   ```
   We shouldn't hide the original exception. Without the exception message and the stacktrace, it's hard to find the root cause when an exception occur.



##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -177,13 +178,9 @@ void initialize(
       GlueClient client,
       LockManager lock,
       FileIO io) {
-    Preconditions.checkArgument(
-        path != null && path.length() > 0,
-        "Cannot initialize GlueCatalog because warehousePath must not be null or empty");
-
     this.catalogName = name;
     this.awsProperties = properties;
-    this.warehousePath = LocationUtil.stripTrailingSlash(path);
+    this.warehousePath = path != null ? LocationUtil.stripTrailingSlash(path) : null;

Review Comment:
   If the `path` is empty, it fails in `LocationUtil.stripTrailiingSlash(path)`
   ```suggestion
       this.warehousePath = (path != null && path.length() > 0) ? LocationUtil.stripTrailingSlash(path) : null;
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org