[#372] fix(hive): Throw exception while column position not found (#487)

### What changes were proposed in this pull request? Throw exception instead of return first position while column position not found ### Why are the changes needed? Clarify the result of column position not found Fix: #372 ### Does this PR introduce _any_ user-facing change? yes, user should specify the existing column position ### How was this patch tested? UTs added
apache · Oct 11, 2023 · eee4361 · eee4361
1 parent 6b3f156
commit eee4361
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 8 deletions.
diff --git a/...atalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/...atalog-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<FieldSchema> 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<FieldSchema> 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
@@ -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()