From eee4361ae276ae5e26ed419c18605a3f39019483 Mon Sep 17 00:00:00 2001
From: mchades <liminghuang@datastrato.com>
Date: Wed, 11 Oct 2023 16:10:18 +0800
Subject: [PATCH] [#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
---
 .../catalog/hive/HiveCatalogOperations.java   | 26 +++++++++----
 .../graviton/catalog/hive/TestHiveTable.java  | 38 +++++++++++++++++++
 2 files changed, 56 insertions(+), 8 deletions(-)

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<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
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()