diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index 2d32f07d8bd..3aa541205ff 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -545,7 +545,8 @@ public Table alterTable(NameIdentifier tableIdent, TableChange... changes) } } else { throw new IllegalArgumentException( - "Unsupported table change type: " + change.getClass().getSimpleName()); + "Unsupported table change type: " + + (change == null ? "null" : change.getClass().getSimpleName())); } } @@ -564,20 +565,26 @@ public Table alterTable(NameIdentifier tableIdent, TableChange... changes) } catch (TException | InterruptedException e) { throw new RuntimeException( "Failed to alter Hive table " + tableIdent.name() + " in Hive metastore", e); + } catch (IllegalArgumentException e) { + throw e; } catch (Exception e) { throw new RuntimeException(e); } } private int columnPosition(List columns, TableChange.ColumnPosition position) { - if (position == null) { - // add to the end by default - return columns.size(); - } else if (position instanceof TableChange.After) { + Preconditions.checkArgument(position != null, "Column position cannot be null"); + if (position instanceof TableChange.After) { String afterColumn = ((TableChange.After) position).getColumn(); - return indexOfColumn(columns, afterColumn) + 1; + int indexOfColumn = indexOfColumn(columns, afterColumn); + Preconditions.checkArgument(indexOfColumn != -1, "Column does not exist: " + afterColumn); + return indexOfColumn + 1; + } else if (position instanceof TableChange.First) { + return 0; + } else { + throw new UnsupportedOperationException( + "Unsupported column position type: " + position.getClass().getSimpleName()); } - return 0; } /** @@ -619,8 +626,11 @@ private void doRemoveProperty( } void doAddColumn(List cols, TableChange.AddColumn change) { + // add to the end by default + int targetPosition = + change.getPosition() == null ? cols.size() : columnPosition(cols, change.getPosition()); cols.add( - columnPosition(cols, change.getPosition()), + targetPosition, new FieldSchema( change.fieldNames()[0], change.getDataType().accept(ToHiveType.INSTANCE).getQualifiedName(), diff --git a/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveTable.java b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveTable.java index b1dd66a8599..c67ecc95f1b 100644 --- a/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveTable.java +++ b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveTable.java @@ -374,6 +374,44 @@ public void testAlterHiveTable() throws IOException { sortOrders); Assertions.assertTrue(hiveCatalog.asTableCatalog().tableExists(tableIdentifier)); + // test exception + Throwable exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + hiveCatalog + .asTableCatalog() + .alterTable( + tableIdentifier, + TableChange.updateColumnPosition( + new String[] {"not_exist_col"}, + TableChange.ColumnPosition.after("col_1")))); + Assertions.assertTrue(exception.getMessage().contains("UpdateColumnPosition does not exist")); + + exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + hiveCatalog + .asTableCatalog() + .alterTable( + tableIdentifier, + TableChange.updateColumnPosition( + new String[] {"col_1"}, + TableChange.ColumnPosition.after("not_exist_col")))); + Assertions.assertTrue(exception.getMessage().contains("Column does not exist")); + + exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + hiveCatalog + .asTableCatalog() + .alterTable( + tableIdentifier, + TableChange.updateColumnPosition(new String[] {"col_1"}, null))); + Assertions.assertTrue(exception.getMessage().contains("Column position cannot be null")); + // test alter hiveCatalog .asTableCatalog()