Address Eduard's feedback: Add unit tests and enable tests across Spark versions

sfc-gh-prsingh · sfc-gh-prsingh · commit 1f1eb11082d9 · 2026-02-11T10:32:39.000-08:00
- Add unit tests in api module for string-to-binary and string-to-fixed conversions - Test valid hex string conversions (uppercase and lowercase) - Test invalid hex strings return null - Test fixed type with wrong length returns null - Remove @disabled testBinaryInFilter from Spark v3.4, v3.5, and v4.0 - The fix in Literals.java now properly handles binary/fixed types - Tests should work across all Spark versions
diff --git a/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java b/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java
@@ -107,6 +107,62 @@ public void testFixedToBinary() {
         .isEqualTo(lit.value().duplicate());
   }
 
+  @Test
+  public void testStringToFixed() {
+    // Test valid hex string to fixed conversion
+    Literal<CharSequence> hexString = Literal.of("000102");
+    Literal<ByteBuffer> fixedLit = hexString.to(Types.FixedType.ofLength(3));
+    assertThat(fixedLit).as("Should convert valid hex string to fixed").isNotNull();
+    assertThat(fixedLit.value().array())
+        .as("Should decode hex string correctly")
+        .isEqualTo(new byte[] {0, 1, 2});
+
+    // Test lowercase hex string
+    Literal<CharSequence> lowercaseHex = Literal.of("0a0b0c");
+    Literal<ByteBuffer> lowercaseFixed = lowercaseHex.to(Types.FixedType.ofLength(3));
+    assertThat(lowercaseFixed).as("Should convert lowercase hex string to fixed").isNotNull();
+    assertThat(lowercaseFixed.value().array())
+        .as("Should decode lowercase hex string correctly")
+        .isEqualTo(new byte[] {10, 11, 12});
+
+    // Test wrong length returns null
+    Literal<CharSequence> wrongLength = Literal.of("0001");
+    assertThat(wrongLength.to(Types.FixedType.ofLength(3)))
+        .as("Should return null for wrong length")
+        .isNull();
+
+    // Test invalid hex string returns null
+    Literal<CharSequence> invalidHex = Literal.of("GGHHII");
+    assertThat(invalidHex.to(Types.FixedType.ofLength(3)))
+        .as("Should return null for invalid hex string")
+        .isNull();
+  }
+
+  @Test
+  public void testStringToBinary() {
+    // Test valid hex string to binary conversion
+    Literal<CharSequence> hexString = Literal.of("000102");
+    Literal<ByteBuffer> binaryLit = hexString.to(Types.BinaryType.get());
+    assertThat(binaryLit).as("Should convert valid hex string to binary").isNotNull();
+    assertThat(binaryLit.value().array())
+        .as("Should decode hex string correctly")
+        .isEqualTo(new byte[] {0, 1, 2});
+
+    // Test lowercase hex string
+    Literal<CharSequence> lowercaseHex = Literal.of("0a0b0c");
+    Literal<ByteBuffer> lowercaseBinary = lowercaseHex.to(Types.BinaryType.get());
+    assertThat(lowercaseBinary).as("Should convert lowercase hex string to binary").isNotNull();
+    assertThat(lowercaseBinary.value().array())
+        .as("Should decode lowercase hex string correctly")
+        .isEqualTo(new byte[] {10, 11, 12});
+
+    // Test invalid hex string returns null
+    Literal<CharSequence> invalidHex = Literal.of("GGHHII");
+    assertThat(invalidHex.to(Types.BinaryType.get()))
+        .as("Should return null for invalid hex string")
+        .isNull();
+  }
+
   @Test
   public void testInvalidBooleanConversions() {
     testInvalidConversions(
diff --git a/spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java b/spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java
@@ -25,8 +25,6 @@
 import org.apache.iceberg.rest.RESTCatalogProperties;
 import org.apache.iceberg.spark.SparkCatalogConfig;
 import org.apache.iceberg.spark.sql.TestSelect;
-import org.junit.jupiter.api.Disabled;
-import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtendWith;
 
 @ExtendWith(ParameterizedTestExtension.class)
@@ -48,11 +46,4 @@ protected static Object[][] parameters() {
       }
     };
   }
-
-  @TestTemplate
-  @Disabled(
-      "binary filter that is used by Spark is not working because ExpressionParser.fromJSON doesn't have the Schema to properly parse the filter expression")
-  public void testBinaryInFilter() {
-    super.testBinaryInFilter();
-  }
 }
diff --git a/spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java b/spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
@@ -629,6 +629,29 @@ public void testBinaryInFilter() {
         sql("SELECT id, binary FROM %s where binary > X'11'", binaryTableName));
   }
 
+  @TestTemplate
+  public void testFixedInFilter() {
+    // Create table programmatically with fixed type since Spark SQL DDL doesn't support it
+    Schema schema =
+        new Schema(
+            Types.NestedField.required(1, "id", Types.LongType.get()),
+            Types.NestedField.required(2, "fixed", Types.FixedType.ofLength(2)));
+
+    TableIdentifier fixedTableIdent = TableIdentifier.of(tableIdent.namespace(), "fixed_table");
+    validationCatalog.createTable(fixedTableIdent, schema, PartitionSpec.unpartitioned());
+
+    String fixedTableName = tableName("fixed_table");
+    sql("INSERT INTO %s VALUES (1, X'0000'), (2, X'1111'), (3, X'0011')", fixedTableName);
+    List<Object[]> expected = ImmutableList.of(row(2L, new byte[] {0x11, 0x11}));
+
+    assertEquals(
+        "Should return all expected rows",
+        expected,
+        sql("SELECT id, fixed FROM %s WHERE fixed > X'0011'", fixedTableName));
+
+    sql("DROP TABLE IF EXISTS %s", fixedTableName);
+  }
+
   @TestTemplate
   public void testComplexTypeFilter() {
     String complexTypeTableName = tableName("complex_table");
diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java
@@ -25,8 +25,6 @@
 import org.apache.iceberg.rest.RESTCatalogProperties;
 import org.apache.iceberg.spark.SparkCatalogConfig;
 import org.apache.iceberg.spark.sql.TestSelect;
-import org.junit.jupiter.api.Disabled;
-import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtendWith;
 
 @ExtendWith(ParameterizedTestExtension.class)
@@ -48,11 +46,4 @@ protected static Object[][] parameters() {
       }
     };
   }
-
-  @TestTemplate
-  @Disabled(
-      "binary filter that is used by Spark is not working because ExpressionParser.fromJSON doesn't have the Schema to properly parse the filter expression")
-  public void testBinaryInFilter() {
-    super.testBinaryInFilter();
-  }
 }
diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
@@ -629,6 +629,29 @@ public void testBinaryInFilter() {
         sql("SELECT id, binary FROM %s where binary > X'11'", binaryTableName));
   }
 
+  @TestTemplate
+  public void testFixedInFilter() {
+    // Create table programmatically with fixed type since Spark SQL DDL doesn't support it
+    Schema schema =
+        new Schema(
+            Types.NestedField.required(1, "id", Types.LongType.get()),
+            Types.NestedField.required(2, "fixed", Types.FixedType.ofLength(2)));
+
+    TableIdentifier fixedTableIdent = TableIdentifier.of(tableIdent.namespace(), "fixed_table");
+    validationCatalog.createTable(fixedTableIdent, schema, PartitionSpec.unpartitioned());
+
+    String fixedTableName = tableName("fixed_table");
+    sql("INSERT INTO %s VALUES (1, X'0000'), (2, X'1111'), (3, X'0011')", fixedTableName);
+    List<Object[]> expected = ImmutableList.of(row(2L, new byte[] {0x11, 0x11}));
+
+    assertEquals(
+        "Should return all expected rows",
+        expected,
+        sql("SELECT id, fixed FROM %s WHERE fixed > X'0011'", fixedTableName));
+
+    sql("DROP TABLE IF EXISTS %s", fixedTableName);
+  }
+
   @TestTemplate
   public void testComplexTypeFilter() {
     String complexTypeTableName = tableName("complex_table");
diff --git a/spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java b/spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java
@@ -25,8 +25,6 @@
 import org.apache.iceberg.rest.RESTCatalogProperties;
 import org.apache.iceberg.spark.SparkCatalogConfig;
 import org.apache.iceberg.spark.sql.TestSelect;
-import org.junit.jupiter.api.Disabled;
-import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtendWith;
 
 @ExtendWith(ParameterizedTestExtension.class)
@@ -48,11 +46,4 @@ protected static Object[][] parameters() {
       }
     };
   }
-
-  @TestTemplate
-  @Disabled(
-      "binary filter that is used by Spark is not working because ExpressionParser.fromJSON doesn't have the Schema to properly parse the filter expression")
-  public void testBinaryInFilter() {
-    super.testBinaryInFilter();
-  }
 }
diff --git a/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java b/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
@@ -638,6 +638,29 @@ public void testBinaryInFilter() {
         sql("SELECT id, binary FROM %s where binary > X'11'", binaryTableName));
   }
 
+  @TestTemplate
+  public void testFixedInFilter() {
+    // Create table programmatically with fixed type since Spark SQL DDL doesn't support it
+    Schema schema =
+        new Schema(
+            Types.NestedField.required(1, "id", Types.LongType.get()),
+            Types.NestedField.required(2, "fixed", Types.FixedType.ofLength(2)));
+
+    TableIdentifier fixedTableIdent = TableIdentifier.of(tableIdent.namespace(), "fixed_table");
+    validationCatalog.createTable(fixedTableIdent, schema, PartitionSpec.unpartitioned());
+
+    String fixedTableName = tableName("fixed_table");
+    sql("INSERT INTO %s VALUES (1, X'0000'), (2, X'1111'), (3, X'0011')", fixedTableName);
+    List<Object[]> expected = ImmutableList.of(row(2L, new byte[] {0x11, 0x11}));
+
+    assertEquals(
+        "Should return all expected rows",
+        expected,
+        sql("SELECT id, fixed FROM %s WHERE fixed > X'0011'", fixedTableName));
+
+    sql("DROP TABLE IF EXISTS %s", fixedTableName);
+  }
+
   @TestTemplate
   public void testComplexTypeFilter() {
     String complexTypeTableName = tableName("complex_table");

Original file line number	Diff line number	Diff line change
`@@ -25,8 +25,6 @@`
`25`	`25`	`import org.apache.iceberg.rest.RESTCatalogProperties;`
`26`	`26`	`import org.apache.iceberg.spark.SparkCatalogConfig;`
`27`	`27`	`import org.apache.iceberg.spark.sql.TestSelect;`
`28`		`-import org.junit.jupiter.api.Disabled;`
`29`		`-import org.junit.jupiter.api.TestTemplate;`
`30`	`28`	`import org.junit.jupiter.api.extension.ExtendWith;`
`31`	`29`
`32`	`30`	`@ExtendWith(ParameterizedTestExtension.class)`
`@@ -48,11 +46,4 @@ protected static Object[][] parameters() {`
`48`	`46`	`}`
`49`	`47`	`};`
`50`	`48`	`}`
`51`		`-`
`52`		`- @TestTemplate`
`53`		`- @Disabled(`
`54`		`- "binary filter that is used by Spark is not working because ExpressionParser.fromJSON doesn't have the Schema to properly parse the filter expression")`
`55`		`- public void testBinaryInFilter() {`
`56`		`- super.testBinaryInFilter();`
`57`		`- }`
`58`	`49`	`}`