Merge pull request #449 from InseeFr/fix/comparison

Fix comparison type issue (Fix #447)

Author: NicoLaval

vtl-engine/src/main/java/fr/insee/vtl/engine/utils/TypeChecking.java

@@ -3,6 +3,7 @@
 import static fr.insee.vtl.engine.VtlScriptEngine.fromContext;
 
 import fr.insee.vtl.engine.exceptions.VtlRuntimeException;
+import fr.insee.vtl.model.Dataset;
 import fr.insee.vtl.model.ResolvableExpression;
 import fr.insee.vtl.model.TypedExpression;
 import fr.insee.vtl.model.exceptions.InvalidTypeException;
@@ -119,6 +120,28 @@ public static boolean hasSameTypeOrNull(List<ResolvableExpression> expressions)
         <= 1;
   }
 
+  /**
+   * Checks if expressions have the same type, all are numbers, or all are null.
+   *
+   * @param expressions List of resolvable expressions to check.
+   * @return A boolean which is <code>true</code> if the expressions have the same type, are all
+   *     numbers, or are all null, <code>false</code> otherwise.
+   */
+  public static boolean hasSameTypeOrNumberOrNull(List<ResolvableExpression> expressions) {
+    var types =
+        expressions.stream()
+            .map(ResolvableExpression::getType)
+            .filter(
+                clazz ->
+                    clazz != null && !Object.class.equals(clazz) && !clazz.equals(Dataset.class))
+            .distinct()
+            .toList();
+
+    return types.isEmpty()
+        || types.stream().allMatch(Number.class::isAssignableFrom)
+        || types.size() == 1; // même type
+  }
+
   /**
    * Checks if an expression evaluates to null. The check is based on the fact that {@link
    * TypedExpression#getType()} returns <code>Object</code> if the expression evaluates to null

vtl-engine/src/main/java/fr/insee/vtl/engine/visitors/expression/ComparisonVisitor.java

@@ -4,6 +4,7 @@
 
 import fr.insee.vtl.engine.exceptions.ConflictingTypesException;
 import fr.insee.vtl.engine.exceptions.VtlRuntimeException;
+import fr.insee.vtl.engine.utils.TypeChecking;
 import fr.insee.vtl.engine.visitors.expression.functions.GenericFunctionsVisitor;
 import fr.insee.vtl.model.ListExpression;
 import fr.insee.vtl.model.Positioned;
@@ -12,11 +13,7 @@
 import fr.insee.vtl.model.exceptions.VtlScriptException;
 import fr.insee.vtl.parser.VtlBaseVisitor;
 import fr.insee.vtl.parser.VtlParser;
-import java.util.Date;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
+import java.util.*;
 import java.util.stream.Collectors;
 import org.antlr.v4.runtime.Token;
 import org.antlr.v4.runtime.tree.TerminalNode;
@@ -138,7 +135,13 @@ public ResolvableExpression visitComparisonExpr(VtlParser.ComparisonExprContext
     try {
       Token type = ((TerminalNode) ctx.op.getChild(0)).getSymbol();
       var leftExpression = exprVisitor.visit(ctx.left);
-      List<ResolvableExpression> parameters = List.of(leftExpression, exprVisitor.visit(ctx.right));
+      var rightExpression = exprVisitor.visit(ctx.right);
+      List<ResolvableExpression> parameters = List.of(leftExpression, rightExpression);
+      // Check 2 parameters have the same types
+      if (!TypeChecking.hasSameTypeOrNumberOrNull(parameters)) {
+        var types = List.of(leftExpression.getType(), rightExpression.getType());
+        throw new ConflictingTypesException(types, fromContext(ctx));
+      }
       // If a parameter is the null token
       if (parameters.stream().map(TypedExpression::getType).anyMatch(Object.class::equals)) {
         return ResolvableExpression.withType(Boolean.class)

vtl-engine/src/test/java/fr/insee/vtl/engine/utils/TypeCheckingUtilsTest.java

@@ -0,0 +1,70 @@
+package fr.insee.vtl.engine.utils;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import fr.insee.vtl.model.Positioned;
+import fr.insee.vtl.model.ResolvableExpression;
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+public class TypeCheckingUtilsTest {
+
+  Positioned fakePos =
+      new Positioned() {
+        @Override
+        public Position getPosition() {
+          return new Position(1, 1, 1, 1);
+        }
+      };
+
+  private ResolvableExpression expr(Class<?> type) {
+    return ResolvableExpression.withType(type)
+        // return fakePos, value is not read
+        .withPosition(fakePos)
+        // return always null, value is not read
+        .using(c -> null);
+  }
+
+  @Test
+  void testAllNull() {
+    var list = List.of(expr(null), expr(null));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testAllSameType() {
+    var list = List.of(expr(String.class), expr(String.class));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testDifferentTypes() {
+    var list = List.of(expr(String.class), expr(Integer.class));
+    assertFalse(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testAllNumbersSameSubclass() {
+    var list = List.of(expr(Integer.class), expr(Integer.class));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testAllNumbersDifferentSubclass() {
+    var list = List.of(expr(Integer.class), expr(Double.class), expr(Long.class));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testMixNullAndNumbers() {
+    var list = List.of(expr(null), expr(Integer.class), expr(Double.class));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+
+  @Test
+  void testMixNullAndOtherType() {
+    var list = List.of(expr(null), expr(String.class));
+    assertTrue(TypeChecking.hasSameTypeOrNumberOrNull(list));
+  }
+}

vtl-spark/src/test/java/fr/insee/vtl/spark/processing.engine/FilterTest.java

@@ -1,8 +1,10 @@
 package fr.insee.vtl.spark.processing.engine;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import fr.insee.vtl.engine.VtlScriptEngine;
+import fr.insee.vtl.engine.exceptions.ConflictingTypesException;
 import fr.insee.vtl.model.Dataset;
 import fr.insee.vtl.model.InMemoryDataset;
 import fr.insee.vtl.model.Structured;
@@ -53,6 +55,22 @@ public void tearDown() {
     if (spark != null) spark.close();
   }
 
+  @Test
+  public void testLessThanBadTypes() {
+    ScriptContext context = engine.getContext();
+    context.setAttribute("ds", dataset, ScriptContext.ENGINE_SCOPE);
+    assertThatThrownBy(() -> engine.eval("ds_out := ds[filter name < 1];"))
+        .isInstanceOf(ConflictingTypesException.class)
+        .hasMessage("conflicting types: [String, Long]");
+  }
+
+  @Test
+  public void testLessThanGoodTypes() throws ScriptException {
+    ScriptContext context = engine.getContext();
+    context.setAttribute("ds", dataset, ScriptContext.ENGINE_SCOPE);
+    engine.eval("ds_out := ds[filter 18.1 < age];");
+  }
+
   @Test
   public void testFilterClause() throws ScriptException {