enforce null literal is nullable

Author: benbellick

core/src/main/java/io/substrait/expression/Expression.java

@@ -53,6 +53,26 @@ <R, C extends VisitationContext, E extends Throwable> R accept(
   abstract class NullLiteral implements Literal {
     public abstract Type type();
 
+    /** A null literal is inherently nullable - you can't have a null of a non-nullable type. */
+    @Override
+    public boolean nullable() {
+      return true;
+    }
+
+    /** Returns this literal unchanged since null literals are always nullable. */
+    @Override
+    public NullLiteral withNullable(boolean nullable) {
+      return this;
+    }
+
+    @Value.Check
+    protected void check() {
+      if (!type().nullable()) {
+        throw new IllegalArgumentException(
+            "NullLiteral requires a nullable type, but got: " + type());
+      }
+    }
+
     @Override
     public Type getType() {
       return type();

core/src/test/java/io/substrait/type/proto/LiteralRoundtripTest.java

@@ -2,6 +2,7 @@
 
 import static io.substrait.expression.proto.ProtoExpressionConverter.EMPTY_TYPE;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.google.protobuf.Any;
 import io.substrait.TestBase;
@@ -287,4 +288,13 @@ void userDefinedLiteralWithAllParameterTypes() {
 
     verifyNestedTypesRoundTrip(multiParam);
   }
+
+  @Test
+  void nullLiteralRejectsNonNullableType() {
+    io.substrait.type.Type nonNullableType =
+        io.substrait.type.Type.I32.builder().nullable(false).build();
+
+    assertThrows(
+        IllegalArgumentException.class, () -> ExpressionCreator.typedNull(nonNullableType));
+  }
 }

isthmus/src/main/java/io/substrait/isthmus/expression/CallConverters.java

@@ -146,11 +146,11 @@ public class CallConverters {
           Expression.Literal lit = (Expression.Literal) operands.get(i);
           boolean fieldIsNullable = fieldTypes.get(i).getType().isNullable();
 
-          // ROW types are never nullable (struct literals are always concrete values).
-          // Field nullability comes from individual field types.
-          if (fieldIsNullable && !lit.nullable()) {
-            lit = lit.withNullable(true);
-          }
+          // Calcite's RexBuilder.makeLiteral() always strips nullability from literal types,
+          // because a concrete value (like 42) is never null. However, Substrait tracks
+          // nullability at the field level to indicate what values that position can hold.
+          // We restore the correct nullability from the ROW's field type schema.
+          lit = lit.withNullable(fieldIsNullable);
           literals.add(lit);
         }
 

isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java

@@ -419,10 +419,7 @@ void tStructRoundtripMixedFieldNullability() {
   void tStructRoundtripWithNullFieldValues() {
     // Test struct with actual NULL field values roundtrips correctly
     Expression.NullLiteral nullField =
-        Expression.NullLiteral.builder()
-            .nullable(true)
-            .type(io.substrait.type.Type.I32.builder().nullable(true).build())
-            .build();
+        ExpressionCreator.typedNull(io.substrait.type.Type.I32.builder().nullable(true).build());
 
     Expression.StructLiteral struct =
         ExpressionCreator.struct(false, nullField, ExpressionCreator.i32(false, 100));

isthmus/src/test/java/io/substrait/isthmus/UserDefinedLiteralRoundtripTest.java

@@ -270,9 +270,15 @@ void parameterizedUdtAllParamKindsRoundTrip() {
     assertRoundTrip(literal);
   }
 
+  /**
+   * Test field-level nullability: struct is non-nullable, but fields are nullable.
+   *
+   * <p>Note: The YAML schema declares point fields as non-nullable (i32), but we're using nullable
+   * fields here. This works because we don't currently validate against the schema. See
+   * https://github.com/substrait-io/substrait-java/issues/614
+   */
   @Test
   void nullableFieldsInStructUdtRoundTrip() {
-    // Test field-level nullability: struct is non-nullable, but fields are nullable
     Expression.UserDefinedStructLiteral literal =
         ExpressionCreator.userDefinedLiteralStruct(
             false,
@@ -286,10 +292,16 @@ void nullableFieldsInStructUdtRoundTrip() {
     assertRoundTrip(literal);
   }
 
+  /**
+   * Test mixed field nullability: struct is non-nullable, first field nullable, second
+   * non-nullable.
+   *
+   * <p>Note: The YAML schema declares point fields as non-nullable (i32), but we're using a
+   * nullable field here. This works because we don't currently validate against the schema. See
+   * https://github.com/substrait-io/substrait-java/issues/614
+   */
   @Test
   void mixedFieldNullabilityInStructUdtRoundTrip() {
-    // Test mixed field nullability: struct is non-nullable, first field nullable, second
-    // non-nullable
     Expression.UserDefinedStructLiteral literal =
         ExpressionCreator.userDefinedLiteralStruct(
             false,
@@ -303,9 +315,9 @@ void mixedFieldNullabilityInStructUdtRoundTrip() {
     assertRoundTrip(literal);
   }
 
+  /** Test struct-level nullability: struct is nullable, fields are non-nullable. */
   @Test
   void nullableStructEncodedUdtRoundTrip() {
-    // Test struct-level nullability: struct is nullable, fields are non-nullable
     Expression.UserDefinedStructLiteral literal =
         ExpressionCreator.userDefinedLiteralStruct(
             true,
@@ -319,9 +331,15 @@ void nullableStructEncodedUdtRoundTrip() {
     assertRoundTrip(literal);
   }
 
+  /**
+   * Test the critical case: nullable struct with mixed field nullability.
+   *
+   * <p>Note: The YAML schema declares point fields as non-nullable (i32), but we're using a
+   * nullable field here. This works because we don't currently validate against the schema. See
+   * https://github.com/substrait-io/substrait-java/issues/614
+   */
   @Test
   void nullableStructWithMixedFieldNullabilityRoundTrip() {
-    // Test the critical case: nullable struct with mixed field nullability
     Expression.UserDefinedStructLiteral literal =
         ExpressionCreator.userDefinedLiteralStruct(
             true,
@@ -396,4 +414,50 @@ void listAndMapFieldsInStructUdtRoundTrip() {
 
     assertRoundTrip(literal);
   }
+
+  /**
+   * Test with an actual null value (NullLiteral) in a struct field.
+   *
+   * <p>Note: The YAML schema declares point fields as non-nullable (i32), but we're using nullable
+   * fields here. This works because we don't currently validate against the schema. When
+   * https://github.com/substrait-io/substrait-java/issues/614 is implemented, this test will need
+   * to be updated to use a UDT with nullable fields in its schema.
+   */
+  @Test
+  void nullValueInStructUdtFieldRoundTrip() {
+    Expression.UserDefinedStructLiteral literal =
+        ExpressionCreator.userDefinedLiteralStruct(
+            false,
+            NESTED_TYPES_URN,
+            "point",
+            Collections.emptyList(),
+            Arrays.asList(
+                ExpressionCreator.typedNull(Type.I32.builder().nullable(true).build()),
+                ExpressionCreator.i32(true, 100)));
+
+    assertRoundTrip(literal);
+  }
+
+  /**
+   * Test with a mix of null and non-null values in struct fields.
+   *
+   * <p>Note: The YAML schema declares point fields as non-nullable (i32), but we're using nullable
+   * fields here. This works because we don't currently validate against the schema. When
+   * https://github.com/substrait-io/substrait-java/issues/614 is implemented, this test will need
+   * to be updated to use a UDT with nullable fields in its schema.
+   */
+  @Test
+  void mixedNullAndNonNullValuesInStructUdtRoundTrip() {
+    Expression.UserDefinedStructLiteral literal =
+        ExpressionCreator.userDefinedLiteralStruct(
+            false,
+            NESTED_TYPES_URN,
+            "point",
+            Collections.emptyList(),
+            Arrays.asList(
+                ExpressionCreator.i32(true, 42),
+                ExpressionCreator.typedNull(Type.I32.builder().nullable(true).build())));
+
+    assertRoundTrip(literal);
+  }
 }