rebase with main and Address comments [2]

apache · Jan 2, 2025 · 1478c6c · 1478c6c
1 parent 7c29aa1
commit 1478c6c
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 34 deletions.
diff --git a/xtable-aws/src/main/java/org/apache/xtable/glue/GlueCatalogConversionSource.java b/xtable-aws/src/main/java/org/apache/xtable/glue/GlueCatalogConversionSource.java
@@ -20,13 +20,11 @@
 
 import static org.apache.xtable.catalog.CatalogUtils.castToHierarchicalTableIdentifier;
 
-import java.util.Locale;
 import java.util.Properties;
 
 import org.apache.hadoop.conf.Configuration;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Strings;
 
 import org.apache.xtable.catalog.TableFormatUtils;
 import org.apache.xtable.conversion.ExternalCatalogConfig;
@@ -60,28 +58,23 @@ public GlueCatalogConversionSource(GlueCatalogConfig glueCatalogConfig, GlueClie
   }
 
   @Override
-  public SourceTable getSourceTable(CatalogTableIdentifier tableIdentifier) {
-    HierarchicalTableIdentifier tblIdentifier = castToHierarchicalTableIdentifier(tableIdentifier);
+  public SourceTable getSourceTable(CatalogTableIdentifier tblIdentifier) {
+    HierarchicalTableIdentifier tableIdentifier = castToHierarchicalTableIdentifier(tblIdentifier);
     try {
       GetTableResponse response =
           glueClient.getTable(
               GetTableRequest.builder()
                   .catalogId(glueCatalogConfig.getCatalogId())
-                  .databaseName(tblIdentifier.getDatabaseName())
-                  .name(tblIdentifier.getTableName())
+                  .databaseName(tableIdentifier.getDatabaseName())
+                  .name(tableIdentifier.getTableName())
                   .build());
       Table table = response.table();
       if (table == null) {
-        throw new IllegalStateException(String.format("table: %s is null", tableIdentifier));
-      }
-
-      String tableFormat = TableFormatUtils.getTableFormat(table.parameters());
-      if (Strings.isNullOrEmpty(tableFormat)) {
         throw new IllegalStateException(
-            String.format("TableFormat is null or empty for table: %s", tableIdentifier.getId()));
+            String.format("table: %s is null", tableIdentifier.getId()));
       }
-      tableFormat = tableFormat.toUpperCase(Locale.ENGLISH);
 
+      String tableFormat = TableFormatUtils.getTableFormat(table.parameters());
       String tableLocation = table.storageDescriptor().location();
       String dataPath =
           TableFormatUtils.getTableDataLocation(tableFormat, tableLocation, table.parameters());
@@ -96,7 +89,7 @@ public SourceTable getSourceTable(CatalogTableIdentifier tableIdentifier) {
           .additionalProperties(tableProperties)
           .build();
     } catch (GlueException e) {
-      throw new CatalogSyncException("Failed to get table: " + tableIdentifier, e);
+      throw new CatalogSyncException("Failed to get table: " + tableIdentifier.getId(), e);
     }
   }
 

diff --git a/xtable-aws/src/test/java/org/apache/xtable/glue/TestGlueCatalogConversionSource.java b/xtable-aws/src/test/java/org/apache/xtable/glue/TestGlueCatalogConversionSource.java
@@ -79,8 +79,15 @@ void testGetSourceTable_errorGettingTableFromGlue() {
     // error getting table from glue
     when(mockGlueClient.getTable(getTableRequest))
         .thenThrow(GlueException.builder().message("something went wrong").build());
-    assertThrows(
-        CatalogSyncException.class, () -> catalogConversionSource.getSourceTable(tableIdentifier));
+    CatalogSyncException exception =
+        assertThrows(
+            CatalogSyncException.class,
+            () -> catalogConversionSource.getSourceTable(tableIdentifier));
+    assertEquals(
+        String.format(
+            "Failed to get table: %s.%s",
+            tableIdentifier.getDatabaseName(), tableIdentifier.getTableName()),
+        exception.getMessage());
 
     verify(mockGlueClient, times(1)).getTable(getTableRequest);
   }
@@ -90,23 +97,15 @@ void testGetSourceTable_tableNotFoundInGlue() {
     // table not found in glue
     when(mockGlueClient.getTable(getTableRequest))
         .thenThrow(EntityNotFoundException.builder().message("table not found").build());
-    assertThrows(
-        CatalogSyncException.class, () -> catalogConversionSource.getSourceTable(tableIdentifier));
-
-    verify(mockGlueClient, times(1)).getTable(getTableRequest);
-  }
-
-  @Test
-  void testGetSourceTable_tableFormatNotPresent() {
-    // table format not present in table properties
-    when(mockGlueClient.getTable(getTableRequest))
-        .thenReturn(GetTableResponse.builder().table(Table.builder().build()).build());
-    IllegalStateException exception =
+    CatalogSyncException exception =
         assertThrows(
-            IllegalStateException.class,
+            CatalogSyncException.class,
             () -> catalogConversionSource.getSourceTable(tableIdentifier));
     assertEquals(
-        "TableFormat is null or empty for table: glue_db.glue_tbl", exception.getMessage());
+        String.format(
+            "Failed to get table: %s.%s",
+            tableIdentifier.getDatabaseName(), tableIdentifier.getTableName()),
+        exception.getMessage());
 
     verify(mockGlueClient, times(1)).getTable(getTableRequest);
   }

diff --git a/xtable-core/src/main/java/org/apache/xtable/catalog/TableFormatUtils.java b/xtable-core/src/main/java/org/apache/xtable/catalog/TableFormatUtils.java
@@ -21,6 +21,7 @@
 import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP;
 import static org.apache.xtable.catalog.Constants.PROP_SPARK_SQL_SOURCES_PROVIDER;
 
+import java.util.Locale;
 import java.util.Map;
 
 import lombok.AccessLevel;
@@ -80,6 +81,9 @@ public static String getTableFormat(Map<String, String> properties) {
     if (Strings.isNullOrEmpty(tableFormat)) {
       tableFormat = properties.get(PROP_SPARK_SQL_SOURCES_PROVIDER);
     }
-    return tableFormat;
+    if (Strings.isNullOrEmpty(tableFormat)) {
+      throw new IllegalArgumentException("Invalid TableFormat: null or empty");
+    }
+    return tableFormat.toUpperCase(Locale.ENGLISH);
   }
 }
diff --git a/xtable-core/src/test/java/org/apache/xtable/catalog/TestTableFormatUtils.java b/xtable-core/src/test/java/org/apache/xtable/catalog/TestTableFormatUtils.java
@@ -20,7 +20,7 @@
 
 import static org.apache.xtable.catalog.Constants.PROP_SPARK_SQL_SOURCES_PROVIDER;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -85,8 +85,10 @@ void testGetTableDataLocation_Iceberg() {
   void testGetTableFormat() {
     Map<String, String> params = new HashMap<>();
 
-    // table format is null when table type param in not present
-    assertNull(TableFormatUtils.getTableFormat(params));
+    // exception thrown when table format is not present
+    IllegalArgumentException exception =
+        assertThrows(IllegalArgumentException.class, () -> TableFormatUtils.getTableFormat(params));
+    assertEquals("Invalid TableFormat: null or empty", exception.getMessage());
 
     // "table_type" is set
     params.put("table_type", TableFormat.ICEBERG);