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 index dbe22d06..703b3d24 100644 --- 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 index bf3ae651..dfdae364 100644 --- 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 index 9f00fe65..9cdea14a 100644 --- 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 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 index 6a0808ef..d09e6b00 100644 --- 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 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);