Improve unit test coverage on validation logics

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Author: yuancu

core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java

@@ -35,6 +35,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import lombok.Getter;
 import org.apache.calcite.jdbc.JavaTypeFactoryImpl;
 import org.apache.calcite.rel.type.RelDataType;
@@ -332,7 +333,8 @@ public Type getJavaClass(RelDataType type) {
   @Override
   public @Nullable RelDataType leastRestrictive(List<RelDataType> types) {
     // Handle UDTs separately, otherwise the least restrictive type will become VARCHAR
-    if (types.stream().anyMatch(OpenSearchTypeUtil::isUserDefinedType)) {
+    if (types.stream().anyMatch(OpenSearchTypeUtil::isUserDefinedType)
+        && types.stream().allMatch(Objects::nonNull)) {
       int nullCount = 0;
       int anyCount = 0;
       int nullableCount = 0;

core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java

@@ -174,13 +174,12 @@ public void testLeastRestrictive_ipAndBinary_returnsVarchar() {
   public void testLeastRestrictive_ipUdtAndOther_returnsIpUdt() {
     // When IP UDT is mixed with OTHER type (which is used as intermediate for IP)
     RelDataType ipUdt = TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP);
-    RelDataType nullType = TYPE_FACTORY.createSqlType(SqlTypeName.NULL);
+    RelDataType otherType = TYPE_FACTORY.createSqlType(SqlTypeName.OTHER);
 
-    RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(ipUdt, nullType));
+    RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(ipUdt, otherType));
 
     assertNotNull(result);
     assertTrue(OpenSearchTypeUtil.isIp(result));
-    assertTrue(result.isNullable());
   }
 
   @Test
@@ -276,4 +275,33 @@ public void testLeastRestrictive_multipleNulls_returnsNullableUdt() {
     assertTrue(OpenSearchTypeUtil.isDate(result));
     assertTrue(result.isNullable());
   }
+
+  // ==================== leastRestrictive null/empty input tests ====================
+
+  @Test
+  public void testLeastRestrictive_emptyList_throwsIllegalArgumentException() {
+    // Empty list causes IllegalArgumentException from Calcite's base implementation
+    org.junit.jupiter.api.Assertions.assertThrows(
+        IllegalArgumentException.class, () -> TYPE_FACTORY.leastRestrictive(List.of()));
+  }
+
+  @Test
+  public void testLeastRestrictive_nullList_throwsNPE() {
+    // Null list causes NullPointerException
+    org.junit.jupiter.api.Assertions.assertThrows(
+        NullPointerException.class, () -> TYPE_FACTORY.leastRestrictive(null));
+  }
+
+  @Test
+  public void testLeastRestrictive_listWithNullElement_throwsNPE() {
+    RelDataType dateUdt = TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE);
+    // List.of() doesn't allow null elements, so we use a different approach
+    java.util.List<RelDataType> types = new java.util.ArrayList<>();
+    types.add(dateUdt);
+    types.add(null);
+
+    // List containing null element causes NPE
+    org.junit.jupiter.api.Assertions.assertThrows(
+        NullPointerException.class, () -> TYPE_FACTORY.leastRestrictive(types));
+  }
 }

core/src/test/java/org/opensearch/sql/calcite/validate/OpenSearchSparkSqlDialectTest.java

@@ -8,6 +8,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.TYPE_FACTORY;
 
@@ -22,6 +23,7 @@
 import org.apache.calcite.sql.fun.SqlStdOperatorTable;
 import org.apache.calcite.sql.parser.SqlParserPos;
 import org.apache.calcite.sql.pretty.SqlPrettyWriter;
+import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.sql.validate.SqlConformance;
 import org.junit.jupiter.api.Test;
 import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;
@@ -108,4 +110,37 @@ public void testGetConformanceIsLiberal() {
     assertNotNull(conformance);
     assertTrue(conformance.isLiberal());
   }
+
+  @Test
+  public void testGetCastSpecForNonIpType() {
+    // Non-IP types should delegate to parent SparkSqlDialect
+    RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
+
+    SqlNode castSpec = OpenSearchSparkSqlDialect.DEFAULT.getCastSpec(intType);
+
+    // SparkSqlDialect returns a SqlDataTypeSpec for INTEGER type
+    assertNotNull(castSpec);
+    assertInstanceOf(SqlDataTypeSpec.class, castSpec);
+    // It should NOT be the IP-specific spec
+    SqlDataTypeSpec typeSpec = (SqlDataTypeSpec) castSpec;
+    assertEquals("INTEGER", typeSpec.toString());
+  }
+
+  @Test
+  public void testGetCastSpecForVarcharType() {
+    RelDataType varcharType = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR);
+
+    SqlNode castSpec = OpenSearchSparkSqlDialect.DEFAULT.getCastSpec(varcharType);
+
+    // SparkSqlDialect handles VARCHAR specially, returns a SqlDataTypeSpec
+    assertNotNull(castSpec);
+    assertInstanceOf(SqlDataTypeSpec.class, castSpec);
+  }
+
+  @Test
+  public void testGetCastSpecForNullType() {
+    // Null input should throw NullPointerException
+    assertThrows(
+        NullPointerException.class, () -> OpenSearchSparkSqlDialect.DEFAULT.getCastSpec(null));
+  }
 }

core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionRuleTest.java

@@ -42,7 +42,7 @@ public void testVarcharToOtherCoercion() {
     SqlTypeCoercionRule rule = PplTypeCoercionRule.instance();
     Map<SqlTypeName, ImmutableSet<SqlTypeName>> mapping = rule.getTypeMapping();
 
-    // VARCHAR should be coercible from OTHER
+    // VARCHAR and CHAR should be coercible to OTHER (for IP type support)
     ImmutableSet<SqlTypeName> otherCoercions = mapping.get(SqlTypeName.OTHER);
     assertNotNull(otherCoercions);
     assertTrue(otherCoercions.contains(SqlTypeName.VARCHAR));

core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionTest.java

@@ -110,6 +110,7 @@ public void testImplicitCast_preservesNullability() {
     assertNotNull(nullableResult);
     assertNotNull(nonNullableResult);
     assertTrue(nullableResult.isNullable());
+    assertFalse(nonNullableResult.isNullable());
   }
 
   // ==================== commonTypeForBinaryComparison tests ====================

core/src/test/java/org/opensearch/sql/calcite/validate/ValidationUtilsTest.java

@@ -230,4 +230,35 @@ public void testTolerantValidationExceptionNullMessage() {
     Exception e = new RuntimeException();
     assertFalse(ValidationUtils.tolerantValidationException(e));
   }
+
+  @Test
+  public void testTolerantValidationExceptionNullException() {
+    assertThrows(
+        NullPointerException.class, () -> ValidationUtils.tolerantValidationException(null));
+  }
+
+  @Test
+  public void testCreateUDTWithAttributesNullSourceType() {
+    // When sourceType is null, syncAttributes handles it gracefully
+    RelDataType dateUdt =
+        ValidationUtils.createUDTWithAttributes(TYPE_FACTORY, null, ExprUDT.EXPR_DATE);
+    assertNotNull(dateUdt);
+  }
+
+  @Test
+  public void testCreateUDTWithAttributesNullExprUDT() {
+    RelDataType sourceType = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true);
+    assertThrows(
+        NullPointerException.class,
+        () -> ValidationUtils.createUDTWithAttributes(TYPE_FACTORY, sourceType, (ExprUDT) null));
+  }
+
+  @Test
+  public void testCreateUDTWithAttributesNullSqlTypeName() {
+    RelDataType sourceType = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true);
+    assertThrows(
+        NullPointerException.class,
+        () ->
+            ValidationUtils.createUDTWithAttributes(TYPE_FACTORY, sourceType, (SqlTypeName) null));
+  }
 }

core/src/test/java/org/opensearch/sql/calcite/validate/shuttles/SkipRelValidationShuttleTest.java

@@ -170,4 +170,96 @@ public void testWidthBucketWithDateTypeTriggersSkip() {
         SkipRelValidationShuttle.SKIP_CALLS.stream().anyMatch(p -> p.test(widthBucketCall));
     assertTrue(shouldSkip);
   }
+
+  @Test
+  public void testWidthBucketWithNullOperandDoesNotTriggerSkip() {
+    // Create WIDTH_BUCKET call with null literal as first operand
+    RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
+    RexLiteral nullLiteral = rexBuilder.makeNullLiteral(intType);
+    RexLiteral minVal = rexBuilder.makeExactLiteral(BigDecimal.valueOf(0), intType);
+    RexLiteral maxVal = rexBuilder.makeExactLiteral(BigDecimal.valueOf(100), intType);
+    RexLiteral buckets = rexBuilder.makeExactLiteral(BigDecimal.valueOf(10), intType);
+
+    RexCall widthBucketCall =
+        (RexCall)
+            rexBuilder.makeCall(
+                PPLBuiltinOperators.WIDTH_BUCKET, nullLiteral, minVal, maxVal, buckets);
+
+    // Null operand should not trigger skip (it's not a datetime type)
+    boolean shouldSkip =
+        SkipRelValidationShuttle.SKIP_CALLS.stream().anyMatch(p -> p.test(widthBucketCall));
+    assertFalse(shouldSkip);
+  }
+
+  @Test
+  public void testWidthBucketWithEmptyOperandsDoesNotTriggerSkip() {
+    // Create mocked WIDTH_BUCKET call with empty operands
+    RexCall call = mock(RexCall.class);
+    when(call.getOperator()).thenReturn(PPLBuiltinOperators.WIDTH_BUCKET);
+    when(call.getOperands()).thenReturn(List.of());
+
+    // Empty operands should not trigger skip
+    boolean shouldSkip = SkipRelValidationShuttle.SKIP_CALLS.stream().anyMatch(p -> p.test(call));
+    assertFalse(shouldSkip);
+  }
+
+  @Test
+  public void testSingleCaseInGroupByDoesNotTriggerSkip() {
+    // Create mocked LogicalAggregate with single CASE expression in GROUP BY
+    RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
+
+    // Create single CASE expression
+    RexInputRef ageRef = new RexInputRef(0, intType);
+    RexLiteral literal30 = rexBuilder.makeExactLiteral(BigDecimal.valueOf(30), intType);
+
+    RexNode caseExpr =
+        rexBuilder.makeCall(
+            SqlStdOperatorTable.CASE,
+            rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN, ageRef, literal30),
+            rexBuilder.makeLiteral("young"),
+            rexBuilder.makeLiteral("old"));
+
+    // Create non-case expression (just a literal)
+    RexNode nonCaseExpr = rexBuilder.makeExactLiteral(BigDecimal.ONE, intType);
+
+    // Mock LogicalProject
+    LogicalProject project = mock(LogicalProject.class);
+    when(project.getProjects()).thenReturn(List.of(caseExpr, nonCaseExpr));
+
+    // Mock LogicalAggregate
+    LogicalAggregate aggregate = mock(LogicalAggregate.class);
+    when(aggregate.getGroupCount()).thenReturn(2);
+    when(aggregate.getInput()).thenReturn(project);
+
+    // Test the predicate - should NOT trigger skip because only 1 CASE
+    boolean shouldSkip =
+        SkipRelValidationShuttle.SKIP_AGGREGATES.stream().anyMatch(p -> p.test(aggregate));
+    assertFalse(shouldSkip);
+  }
+
+  @Test
+  public void testAggregateWithNonProjectInputDoesNotTriggerSkip() {
+    // Mock LogicalAggregate with non-LogicalProject input
+    LogicalAggregate aggregate = mock(LogicalAggregate.class);
+    LogicalValues values = mock(LogicalValues.class);
+    when(aggregate.getGroupCount()).thenReturn(2);
+    when(aggregate.getInput()).thenReturn(values);
+
+    // Test the predicate - should NOT trigger skip because input is not LogicalProject
+    boolean shouldSkip =
+        SkipRelValidationShuttle.SKIP_AGGREGATES.stream().anyMatch(p -> p.test(aggregate));
+    assertFalse(shouldSkip);
+  }
+
+  @Test
+  public void testAggregateWithSingleGroupDoesNotTriggerSkip() {
+    // Mock LogicalAggregate with single group (groupCount = 1)
+    LogicalAggregate aggregate = mock(LogicalAggregate.class);
+    when(aggregate.getGroupCount()).thenReturn(1);
+
+    // Test the predicate - should NOT trigger skip because only 1 group
+    boolean shouldSkip =
+        SkipRelValidationShuttle.SKIP_AGGREGATES.stream().anyMatch(p -> p.test(aggregate));
+    assertFalse(shouldSkip);
+  }
 }