Repository: squashql/squashql (java)

Issue:
Unify the api of criterion
The API to create a criterion can be confusing and might not always work. For instance, this will work:

```java
criterion(this.date, Functions.currentDate(), ConditionType.LT)
```

but this will not work but it compiles:
```java
criterion(this.date, Functions.lt(Functions.currentDate()))
```

It is at least expected to be working. Maybe keeping only one of them would be simpler.

Pull request description:
Fix criterion inconsistencies
Fixes #309 

Patch:
diff --git a/core/src/main/java/io/squashql/query/QueryResolver.java b/core/src/main/java/io/squashql/query/QueryResolver.java
index 21147ac..157a56c 100644
--- a/core/src/main/java/io/squashql/query/QueryResolver.java
+++ b/core/src/main/java/io/squashql/query/QueryResolver.java
@@ -21,6 +21,7 @@ import java.util.stream.Stream;
 
 import static io.squashql.query.ColumnSetKey.GROUP;
 import static io.squashql.query.compiled.CompiledAggregatedMeasure.COMPILED_COUNT;
+import static io.squashql.query.dto.SingleValueConditionDto.SUPPORTED_TYPES;
 
 @Data
 public class QueryResolver {
@@ -134,10 +135,10 @@ public class QueryResolver {
   }
 
   private TableTypedField getTableTypedField(String fieldName, String alias) {
-    final String[] split = fieldName.split("\\.");
+    String[] split = fieldName.split("\\.");
     if (split.length > 1) {
-      final String tableName = split[0];
-      final String fieldNameInTable = split[1];
+      String tableName = split[0];
+      String fieldNameInTable = split[1];
       Store store = this.storeByName.get(tableName);
       if (store != null) {
         for (TableTypedField field : store.fields()) {
@@ -166,8 +167,8 @@ public class QueryResolver {
 
   private QueryScope toQueryScope(QueryDto query) {
     checkQuery(query);
-    final List<TypedField> columnSets = query.columnSets.values().stream().flatMap(cs -> cs.getColumnsForPrefetching().stream()).map(this::resolveField).toList();
-    final List<TypedField> combinedColumns = Stream.concat(this.columns.stream(), columnSets.stream()).toList();
+    List<TypedField> columnSets = query.columnSets.values().stream().flatMap(cs -> cs.getColumnsForPrefetching().stream()).map(this::resolveField).toList();
+    List<TypedField> combinedColumns = Stream.concat(this.columns.stream(), columnSets.stream()).toList();
 
     List<TypedField> rollup = query.rollupColumns.stream().map(this::resolveField).toList();
     Set<Set<TypedField>> groupingSets = query.groupingSets.stream().map(g -> g.stream().map(this::resolveField).collect(Collectors.toSet())).collect(Collectors.toSet());
@@ -183,7 +184,7 @@ public class QueryResolver {
             query.limit);
   }
 
-  protected void checkQuery(final QueryDto query) {
+  protected void checkQuery(QueryDto query) {
     if (query.table.name == null && query.table.subQuery == null) {
       throw new IllegalArgumentException("A table or sub-query was expected in " + query);
     } else if (query.table.name != null && query.table.subQuery != null) {
@@ -256,18 +257,35 @@ public class QueryResolver {
   /**
    * Criteria
    */
-  public CompiledCriteria compileCriteria(final CriteriaDto criteria) {
+  public CompiledCriteria compileCriteria(CriteriaDto criteria) {
     return criteria == null
             ? null
-            : this.cache.computeIfAbsent(criteria, c -> new CompiledCriteria(c.condition, c.conditionType, c.field == null ? null : resolveWithFallback(c.field), c.fieldOther == null ? null : resolveWithFallback(c.fieldOther),
-            c.measure == null ? null : compileMeasure(c.measure, true),
-            c.children.stream().map(this::compileCriteria).collect(Collectors.toList())));
+            : this.cache.computeIfAbsent(criteria, c -> {
+      if (c.field != null && c.condition instanceof SingleValueConditionDto svc && svc.value instanceof Field other) {
+        // transform the condition
+        return compileCriteria(new CriteriaDto(c.field, other, c.condition.type()));
+      } else if (c.field != null && c.fieldOther != null && c.conditionType != null && SUPPORTED_TYPES.contains(c.conditionType)) {
+        // transform the condition
+        if (c.field instanceof ConstantField cf) {
+          return compileCriteria(new CriteriaDto(c.fieldOther, new SingleValueConditionDto(c.conditionType, cf.value)));
+        } else if (c.fieldOther instanceof ConstantField cf) {
+          return compileCriteria(new CriteriaDto(c.field, new SingleValueConditionDto(c.conditionType, cf.value)));
+        }
+      }
+      return new CompiledCriteria(
+              c.condition,
+              c.conditionType,
+              c.field == null ? null : resolveWithFallback(c.field),
+              c.fieldOther == null ? null : resolveWithFallback(c.fieldOther),
+              c.measure == null ? null : compileMeasure(c.measure, true),
+              c.children.stream().map(this::compileCriteria).collect(Collectors.toList()));
+    });
   }
 
   /**
    * Compiles measures
    */
-  private Map<Measure, CompiledMeasure> compileMeasures(final List<Measure> measures, boolean topMeasures) {
+  private Map<Measure, CompiledMeasure> compileMeasures(List<Measure> measures, boolean topMeasures) {
     return measures
             .stream()
             .collect(Collectors.toMap(Function.identity(), m -> compileMeasure(m, topMeasures)));
@@ -275,7 +293,7 @@ public class QueryResolver {
 
   protected CompiledMeasure compileMeasure(Measure measure, boolean topMeasure) {
     return this.cache.computeIfAbsent(measure, m -> {
-      final CompiledMeasure compiledMeasure;
+      CompiledMeasure compiledMeasure;
       if (m instanceof AggregatedMeasure am) {
         compiledMeasure = compileAggregatedMeasure(am);
       } else if (m instanceof ExpressionMeasure em) {
@@ -432,7 +450,7 @@ public class QueryResolver {
       if (this.compiledMeasure.containsKey(measure)) {
         return this.compiledMeasure.get(measure);
       } else {
-        final CompiledMeasure compiledMeasure = mappingFunction.apply(measure);
+        CompiledMeasure compiledMeasure = mappingFunction.apply(measure);
         this.compiledMeasure.put(measure, compiledMeasure);
         return compiledMeasure;
       }
@@ -442,8 +460,8 @@ public class QueryResolver {
       if (this.compiledCriteria.containsKey(criteria)) {
         return this.compiledCriteria.get(criteria);
       } else {
-        final CompiledCriteria compiledCriteria = mappingFunction.apply(criteria);
-        this.compiledCriteria.computeIfAbsent(criteria, mappingFunction);
+        CompiledCriteria compiledCriteria = mappingFunction.apply(criteria);
+        this.compiledCriteria.put(criteria, compiledCriteria);
         return compiledCriteria;
       }
     }
diff --git a/core/src/main/java/io/squashql/query/dto/SingleValueConditionDto.java b/core/src/main/java/io/squashql/query/dto/SingleValueConditionDto.java
index 0170c57..f89b21d 100644
--- a/core/src/main/java/io/squashql/query/dto/SingleValueConditionDto.java
+++ b/core/src/main/java/io/squashql/query/dto/SingleValueConditionDto.java
@@ -13,14 +13,14 @@ import static io.squashql.query.dto.ConditionType.*;
 @NoArgsConstructor // For Jackson
 public final class SingleValueConditionDto implements ConditionDto {
 
-  private static final EnumSet<ConditionType> supportedTypes = EnumSet.of(LT, LE, GT, GE, EQ, NEQ, LIKE, ARRAY_CONTAINS);
+  public static final EnumSet<ConditionType> SUPPORTED_TYPES = EnumSet.of(LT, LE, GT, GE, EQ, NEQ, LIKE, ARRAY_CONTAINS);
 
   public ConditionType type;
 
   public Object value;
 
   public SingleValueConditionDto(ConditionType type, Object value) {
-    if (!supportedTypes.contains(type)) {
+    if (!SUPPORTED_TYPES.contains(type)) {
       throw new IllegalArgumentException("Unexpected type for SVC: " + type);
     }
     this.type = type;


Test patch:
diff --git a/core/src/test/java/io/squashql/query/TestSQLTranslator.java b/core/src/test/java/io/squashql/query/TestSQLTranslator.java
index 050bd21..ad2c27e 100644
--- a/core/src/test/java/io/squashql/query/TestSQLTranslator.java
+++ b/core/src/test/java/io/squashql/query/TestSQLTranslator.java
@@ -75,7 +75,7 @@ public class TestSQLTranslator {
 
     DatabaseQuery toDatabaseQuery() {
       // Keep the order
-      List<CompiledMeasure> measures = getQuery().measures.stream().map(measure -> getMeasures().get(measure)).filter(m -> m != null).toList();
+      List<CompiledMeasure> measures = getQuery().measures.stream().map(measure -> getMeasures().get(measure)).filter(Objects::nonNull).toList();
       return new DatabaseQuery(getScope().copyWithNewLimit(this.limit), measures);
     }
 
@@ -506,9 +506,9 @@ public class TestSQLTranslator {
 
   @Test
   void testGroupingSets() {
-    final Field a = tableField("a");
-    final Field b = tableField("b");
-    final QueryDto query = new QueryDto()
+    Field a = tableField("a");
+    Field b = tableField("b");
+    QueryDto query = new QueryDto()
             .withColumn(a)
             .withColumn(b)
             .withMeasure(new AggregatedMeasure("pnl.sum", "pnl", "sum"))
@@ -578,4 +578,77 @@ public class TestSQLTranslator {
     // When field is a string, values should be escaped
     assertThat(sql).isEqualTo("case when `dataset.store`.`field` is null then 1 when `dataset.store`.`field` = '2023' then 2 when `dataset.store`.`field` = '2027' then 3 else 4 end");
   }
+
+  @Test
+  void testCriteria() {
+    Field a = tableField("a");
+    Field b = tableField("b");
+    // The following conditions are equivalent
+    CriteriaDto c1 = criterion(a, b, ConditionType.EQ);
+    CriteriaDto c2 = criterion(a, eq(b));
+    QueryDto q1 = Query.from(BASE_STORE_NAME)
+            .where(c1)
+            .select(List.of(a), List.of())
+            .build();
+    QueryDto q2 = Query.from(BASE_STORE_NAME)
+            .where(c2)
+            .select(List.of(a), List.of())
+            .build();
+    assertThat(translate(compileQuery(q1)))
+            .isEqualTo(translate(compileQuery(q2)))
+            .isEqualTo("select `a` from `dataset.baseStore` where `a` = `b` group by `a`");
+  }
+
+  @Test
+  void testCriteriaWithConstant() {
+    Field a = tableField("a"); // it is a String
+    {
+      // The following conditions are equivalent
+      ConstantField cf = new ConstantField("hello");
+      CriteriaDto c1 = criterion(a, cf, ConditionType.EQ);
+      CriteriaDto c1bis = criterion(cf, a, ConditionType.EQ);
+      CriteriaDto c2 = criterion(a, eq(cf));
+      QueryDto q1 = Query.from(BASE_STORE_NAME)
+              .where(c1)
+              .select(List.of(a), List.of())
+              .build();
+      QueryDto q1bis = Query.from(BASE_STORE_NAME)
+              .where(c1bis)
+              .select(List.of(a), List.of())
+              .build();
+      QueryDto q2 = Query.from(BASE_STORE_NAME)
+              .where(c2)
+              .select(List.of(a), List.of())
+              .build();
+      assertThat(translate(compileQuery(q1)))
+              .isEqualTo(translate(compileQuery(q1bis)))
+              .isEqualTo(translate(compileQuery(q2)))
+              .isEqualTo("select `a` from `dataset.baseStore` where `a` = 'hello' group by `a`");
+    }
+
+    {
+      Field pnl = tableField("pnl"); // it is a double
+      // The following conditions are equivalent
+      ConstantField cf = new ConstantField(5);
+      CriteriaDto c1 = criterion(pnl, cf, ConditionType.GT);
+      CriteriaDto c1bis = criterion(cf, pnl, ConditionType.GT);
+      CriteriaDto c2 = criterion(pnl, gt(cf));
+      QueryDto q1 = Query.from(BASE_STORE_NAME)
+              .where(c1)
+              .select(List.of(pnl), List.of())
+              .build();
+      QueryDto q1bis = Query.from(BASE_STORE_NAME)
+              .where(c1bis)
+              .select(List.of(pnl), List.of())
+              .build();
+      QueryDto q2 = Query.from(BASE_STORE_NAME)
+              .where(c2)
+              .select(List.of(pnl), List.of())
+              .build();
+      assertThat(translate(compileQuery(q1)))
+              .isEqualTo(translate(compileQuery(q1bis)))
+              .isEqualTo(translate(compileQuery(q2)))
+              .isEqualTo("select `pnl` from `dataset.baseStore` where `pnl` > 5 group by `pnl`");
+    }
+  }
 }