[GitHub] [iceberg] dramaticlly commented on a diff in pull request #7652: Spark 3.4: Fix NPE when create branch and tag on table without snapshot

["dramaticlly (via GitHub)" <gi...@apache.org>]

dramaticlly commented on code in PR #7652:
URL: https://github.com/apache/iceberg/pull/7652#discussion_r1199158777


##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestBranchDDL.java:
##########
@@ -89,6 +91,13 @@ public void testCreateBranch() throws NoSuchTableException {
         () -> sql("ALTER TABLE %s CREATE BRANCH %s", tableName, branchName));
   }
 
+  @Test
+  public void testCreateBranchOnEmptyTable() {
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE %s CREATE BRANCH %s", tableName, "b1"))
+        .isInstanceOf(ValidationException.class)
+        .hasMessageContaining("Cannot set b1 to unknown snapshot");

Review Comment:
   discussed offline, looks like this is relate to implicit scala Long to primitive java.lang.long conversion, seems fragile and confusion if we rely on that to guard against bad input. So we ended check snapshotId not null before pass to java code and also make unit test more self-contained. 
   
   @amogh-jahagirdar ready for another look, if new exception and error message looks good to you, I'll backport fix to other spark version and try to get it merged for 1.3.0 release



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org