fix!: replace assert-based validation with real exceptions

CONTRIBUTING.md

@@ -77,8 +77,10 @@ CI — build the whole thing before pushing:
 * **PMD** (`substrait.java-conventions`, ruleset
   `build-logic/src/main/resources/substrait-pmd.xml`) runs via `check` / `build` and fails on
   violations. Common tripwires: missing `@Override`, unused private fields/methods/locals,
-  `var` (rule `UseExplicitTypes` — use explicit types), and `public` JUnit 5 test
-  classes/methods (they must be package-private).
+  `var` (rule `UseExplicitTypes` — use explicit types), `assert` (rule `AvoidAssertStatement` —
+  assertions are disabled unless the JVM runs with `-ea`, so throw `IllegalArgumentException`
+  for caller-facing invariants and `IllegalStateException` for internal ones), and `public`
+  JUnit 5 test classes/methods (they must be package-private).
 * **javadoc** doclint fails the build, but only via `build` / `javadocJar` — run
   `./gradlew :core:javadoc` before pushing.
 * **CI** runs the full `./gradlew build --rerun-tasks` plus `yamllint`, `editorconfig-checker`,

build-logic/src/main/resources/substrait-pmd.xml

@@ -21,4 +21,22 @@
   <rule ref="category/java/bestpractices.xml/UnusedPrivateMethod" />
   <rule ref="category/java/design.xml/AvoidThrowingRawExceptionTypes" />
   <rule ref="category/java/errorprone.xml/MissingSerialVersionUID" />
+
+  <rule name="AvoidAssertStatement" language="java"
+    message="Do not validate with assert: assertions only run when the JVM is started with -ea. Throw IllegalArgumentException for caller-facing invariants or IllegalStateException for internal ones."
+    class="net.sourceforge.pmd.lang.rule.xpath.XPathRule">
+    <description>
+      Java assertions are disabled unless the host JVM runs with -ea, which is the case for
+      Gradle's test JVMs and for almost nothing else. An assert-based check is therefore
+      enforced in CI and silently skipped in every real deployment, and AssertionError is an
+      Error rather than an Exception, so callers that catch Exception to report a bad plan
+      never see it.
+    </description>
+    <priority>3</priority>
+    <properties>
+      <property name="xpath">
+        <value>//AssertStatement</value>
+      </property>
+    </properties>
+  </rule>
 </ruleset>

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

@@ -1789,13 +1789,28 @@ abstract class NestedList implements Nested {
      */
     public abstract List<Expression> values();
 
-    /** Validates that the nested list is not empty and all values have the same type. */
+    /**
+     * Validates that the nested list is not empty and all values have the same type, disregarding
+     * nullability. Values of mixed nullability are allowed, because SQL list constructors do not
+     * cast their values to a common type: {@code ARRAY[not_null_column, nullable_column]} produces
+     * values that differ only in nullability. The list's element type is that of its first value,
+     * as {@link #getType()} shows.
+     *
+     * @throws IllegalArgumentException if the list is empty or its values have differing types
+     */
     @Value.Check
     protected void check() {
-      assert !values().isEmpty() : "To specify an empty list, use ExpressionCreator.emptyList()";
+      if (values().isEmpty()) {
+        throw new IllegalArgumentException(
+            "To specify an empty list, use ExpressionCreator.emptyList()");
+      }
 
-      assert values().stream().map(Expression::getType).distinct().count() <= 1
-          : "All values in NestedList must have the same type";
+      List<Type> types =
+          values().stream().map(Expression::getType).collect(java.util.stream.Collectors.toList());
+      if (types.stream().map(TypeCreator::asNullable).distinct().limit(2).count() > 1) {
+        throw new IllegalArgumentException(
+            String.format("All values in NestedList must have the same type, found: %s", types));
+      }
     }
 
     @Override

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

@@ -20,7 +20,7 @@ class VariadicParameterConsistencyValidator {
    *
    * @param func the function declaration
    * @param arguments the function arguments to validate
-   * @throws AssertionError if validation fails
+   * @throws IllegalArgumentException if validation fails
    */
   public static void validate(SimpleExtension.Function func, List<FunctionArg> arguments) {
     Optional<SimpleExtension.VariadicBehavior> variadic = func.variadic();
@@ -85,7 +85,7 @@ public static void validate(SimpleExtension.Function func, List<FunctionArg> arg
     for (int i = firstVariadicArgIdx + 1; i < argumentTypes.size(); i++) {
       Type currentType = argumentTypes.get(i);
       if (!firstVariadicType.equalsIgnoringNullability(currentType)) {
-        throw new AssertionError(
+        throw new IllegalArgumentException(
             String.format(
                 "Variadic arguments must have consistent types when parameterConsistency is CONSISTENT. "
                     + "Argument at index %d has type %s but argument at index %d has type %s",

core/src/main/java/io/substrait/expression/proto/ExpressionProtoConverter.java

@@ -477,7 +477,12 @@ public Expression visit(
 
   private Expression.Literal toLiteral(io.substrait.expression.Expression expression) {
     Expression e = toProto(expression);
-    assert e.getRexTypeCase() == Expression.RexTypeCase.LITERAL;
+    if (e.getRexTypeCase() != Expression.RexTypeCase.LITERAL) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Expected a literal expression, but %s was converted to a %s",
+              expression.getClass().getSimpleName(), e.getRexTypeCase()));
+    }
     return e.getLiteral();
   }
 

core/src/main/java/io/substrait/relation/VirtualTableScan.java

@@ -5,7 +5,6 @@
 import io.substrait.type.Type;
 import io.substrait.util.VisitationContext;
 import java.util.List;
-import java.util.Objects;
 import org.immutables.value.Value;
 
 /** A read relation that produces an inline table from a fixed list of literal rows. */
@@ -23,67 +22,82 @@ public abstract class VirtualTableScan extends AbstractReadRel {
    * Checks the following invariants when construction a VirtualTableScan
    *
    * <ul>
-   *   <li>no null field names
-   *   <li>no null rows
    *   <li>row shape must match field-list
    *   <li>row field types must match schema types
    * </ul>
+   *
+   * @throws IllegalArgumentException if any of these invariants is violated
    */
   @Value.Check
   protected void check() {
     List<String> names = getInitialSchema().names();
 
-    assert names.size()
-        == NamedFieldCountingTypeVisitor.countNames(this.getInitialSchema().struct());
-    List<Expression.NestedStruct> rows = getRows();
-
-    // At the PROTOBUF layer, the Nested.Struct message does not carry nullability information.
-    // Nullability is attached to the Nested message, which can contain a Nested.Struct.
-    // The NestedStruct POJO flattens the Nested and Nested.Struct messages together, allowing the
-    // nullability of a NestedStruct to be set directly.
-    //
-    // HOWEVER, the VirtualTable message contains a list of Nested.Struct messages, and as such
-    // the nullability cannot be set at the protobuf layer. To avoid users attaching meaningless
-    // nullability information in the POJOs, we restrict the nullability of NestedStructs to false
-    // when used in VirtualTableScans.
-    for (Expression.NestedStruct row : rows) {
-      assert !row.nullable();
+    int schemaNameCount =
+        NamedFieldCountingTypeVisitor.countNames(this.getInitialSchema().struct());
+    if (names.size() != schemaNameCount) {
+      throw new IllegalArgumentException(
+          String.format(
+              "VirtualTableScan schema names count (%d) does not match the depth-first named-field count of the schema struct (%d)",
+              names.size(), schemaNameCount));
     }
 
-    assert names.stream().noneMatch(Objects::isNull)
-        && rows.stream().noneMatch(Objects::isNull)
-        && rows.stream()
-            .allMatch(r -> NamedFieldCountingTypeVisitor.countNames(r.getType()) == names.size());
+    List<Type> schemaFieldTypes = getInitialSchema().struct().fields();
+
+    for (Expression.NestedStruct row : getRows()) {
+      // At the PROTOBUF layer, the Nested.Struct message does not carry nullability information.
+      // Nullability is attached to the Nested message, which can contain a Nested.Struct.
+      // The NestedStruct POJO flattens the Nested and Nested.Struct messages together, allowing
+      // the nullability of a NestedStruct to be set directly.
+      //
+      // HOWEVER, the VirtualTable message contains a list of Nested.Struct messages, and as such
+      // the nullability cannot be set at the protobuf layer. To avoid users attaching meaningless
+      // nullability information in the POJOs, we restrict the nullability of NestedStructs to
+      // false when used in VirtualTableScans.
+      if (row.nullable()) {
+        throw new IllegalArgumentException(
+            "VirtualTableScan rows must not be nullable; nullability cannot be represented for the Nested.Struct messages of a VirtualTable");
+      }
 
-    for (Expression.NestedStruct row : rows) {
-      validateRowConformsToSchema(row);
+      int rowNameCount = NamedFieldCountingTypeVisitor.countNames(row.getType());
+      if (rowNameCount != names.size()) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Row named-field count (%d) does not match schema names count (%d)",
+                rowNameCount, names.size()));
+      }
+
+      validateRowConformsToSchema(row, schemaFieldTypes);
     }
   }
 
   /**
    * Validates that a row's field types conform to the table's schema.
    *
    * @param row the row to validate
-   * @throws AssertionError if the row does not conform to the schema
+   * @param schemaFieldTypes the field types of the table's schema
+   * @throws IllegalArgumentException if the row does not conform to the schema
    */
-  private void validateRowConformsToSchema(Expression.NestedStruct row) {
-    Type.Struct schemaStruct = getInitialSchema().struct();
-    List<Type> schemaFieldTypes = schemaStruct.fields();
+  private static void validateRowConformsToSchema(
+      Expression.NestedStruct row, List<Type> schemaFieldTypes) {
     List<Expression> rowFields = row.fields();
 
-    assert rowFields.size() == schemaFieldTypes.size()
-        : String.format(
-            "Row field count (%d) does not match schema field count (%d)",
-            rowFields.size(), schemaFieldTypes.size());
+    if (rowFields.size() != schemaFieldTypes.size()) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Row field count (%d) does not match schema field count (%d)",
+              rowFields.size(), schemaFieldTypes.size()));
+    }
 
     for (int i = 0; i < rowFields.size(); i++) {
       Type rowFieldType = rowFields.get(i).getType();
       Type schemaFieldType = schemaFieldTypes.get(i);
 
-      assert rowFieldType.equals(schemaFieldType)
-          : String.format(
-              "Row field type (%s) does not match schema field type (%s)",
-              rowFieldType, schemaFieldType);
+      if (!rowFieldType.equals(schemaFieldType)) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Row field type (%s) does not match schema field type (%s)",
+                rowFieldType, schemaFieldType));
+      }
     }
   }
 

core/src/test/java/io/substrait/expression/VariadicParameterConsistencyTest.java

@@ -85,7 +85,7 @@ void testConsistentVariadicWithDifferentTypes() {
             .build();
 
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             createScalarFunctionInvocation(
                 args,
@@ -97,7 +97,7 @@ void testConsistentVariadicWithDifferentTypes() {
         "Consistent variadic with different types should fail");
 
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             createScalarFunctionInvocation(
                 args,
@@ -181,7 +181,7 @@ void testConsistentVariadicWithWildcardType() {
         "Consistent variadic with wildcard type and same concrete types should pass");
 
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             createScalarFunctionInvocation(
                 args,
@@ -218,7 +218,7 @@ void testConsistentVariadicWithMinGreaterThanOne() {
         "Consistent variadic with min=2 and same types should pass");
 
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             createScalarFunctionInvocation(
                 args,

core/src/test/java/io/substrait/relation/VirtualTableScanTest.java

@@ -112,7 +112,7 @@ void rejectInvalidRowNullability() {
                     .addFields(ExpressionCreator.i64(true, 1))
                     .nullable(true) // can't have nullable rows
                     .build());
-    assertThrows(AssertionError.class, bldr::build);
+    assertThrows(IllegalArgumentException.class, bldr::build);
   }
 
   @Test
@@ -134,7 +134,7 @@ void checkValidRowsWithSimpleTypes() {
   void checkInvalidRowTypeMismatch() {
     // Row has I32 where schema expects STRING
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             ImmutableVirtualTableScan.builder()
                 .initialSchema(
@@ -153,7 +153,7 @@ void checkInvalidRowTypeMismatch() {
   void checkInvalidRowWrongFieldCount() {
     // Row has wrong number of fields
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             ImmutableVirtualTableScan.builder()
                 .initialSchema(
@@ -169,7 +169,7 @@ false, i64(false, 1L), string(false, "Alice"))) // Missing age field
   void checkInvalidNestedStructTypeMismatch() {
     // Nested struct has wrong field type
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             ImmutableVirtualTableScan.builder()
                 .initialSchema(
@@ -191,7 +191,7 @@ void checkInvalidNestedStructTypeMismatch() {
   void checkInvalidListElementTypeMismatch() {
     // List has wrong element type
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             ImmutableVirtualTableScan.builder()
                 .initialSchema(
@@ -208,7 +208,7 @@ void checkInvalidListElementTypeMismatch() {
   void checkInvalidNullabilityMismatch() {
     // Nullability must match exactly
     assertThrows(
-        AssertionError.class,
+        IllegalArgumentException.class,
         () ->
             ImmutableVirtualTableScan.builder()
                 .initialSchema(

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

@@ -1,10 +1,12 @@
 package io.substrait.type.proto;
 
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import io.substrait.TestBase;
 import io.substrait.expression.Expression;
+import io.substrait.expression.ExpressionCreator;
 import io.substrait.expression.ImmutableExpression;
 import org.junit.jupiter.api.Test;
 
@@ -17,7 +19,7 @@ class NestedListExpressionTest extends TestBase {
   void rejectNestedListWithElementsOfDifferentTypes() {
     ImmutableExpression.NestedList.Builder builder =
         Expression.NestedList.builder().addValues(literalExpression).addValues(sb.i32(12));
-    assertThrows(AssertionError.class, builder::build);
+    assertThrows(IllegalArgumentException.class, builder::build);
   }
 
   @Test
@@ -34,10 +36,30 @@ void acceptNestedListWithElementsOfSameType() {
     verifyRoundTrip(project);
   }
 
+  @Test
+  void acceptNestedListWithElementsOfMixedNullability() {
+    // A list of values that differ only in nullability is valid; SQL builds such lists from
+    // ARRAY[not_null_column, nullable_column].
+    Expression.NestedList mixedNullability =
+        Expression.NestedList.builder()
+            .addValues(sb.i32(12))
+            .addValues(ExpressionCreator.typedNull(N.I32))
+            .build();
+    // The element type is the type of the first value, nullability included.
+    assertEquals(R.list(R.I32), mixedNullability.getType());
+
+    io.substrait.relation.Project project =
+        io.substrait.relation.Project.builder()
+            .addExpressions(mixedNullability)
+            .input(sb.emptyVirtualTableScan())
+            .build();
+    verifyRoundTrip(project);
+  }
+
   @Test
   void rejectEmptyNestedListTest() {
     ImmutableExpression.NestedList.Builder builder = Expression.NestedList.builder();
-    assertThrows(AssertionError.class, builder::build);
+    assertThrows(IllegalArgumentException.class, builder::build);
   }
 
   @Test