Repository: squashql/squashql (java)

Issue:
Join between two sub-queries does not get data from the the second sub-query when a common field is null
If I have two sub queries and I want to join them, squashQL seems to be joining on the common fields, but when the value of one of these common fields is `null` the join doesn't work properly as it returns `null` in the columns resulting from the second subquery.

Here's an example:
```
const salesSumQuery = from(sales._name)
      .select([sales.product, sales.region], [], [sum("sales", sales.revenue)])
      .build();
const salesAvgQuery = from(sales._name)
     .select([sales.product, sales.region], [], [avg("sales", sales.revenue)])
     .build()
const joinQuery = new QueryJoin(salesSumQuery).join(salesAvgQuery, JoinType.LEFT);
```

If `sales.region` is null, then the condition of the join will have `null = null` and this equals `null` and not `true`. The join won't work and I won't get the measure of the second sub query.

A solution would be to take into account null fields in the join condition, for example: `coalesce(column1, ' ') = coalesce(column2, ' ')` or `( column1 = column2 OR (column1 is null and column2 is null))` 

Pull request:
#341 Fix query join criteria when columns in common are null
Closes #341

Clickhouse version in tests has been upgraded because the old one does not support complex join criteria

Patch:
diff --git a/core/src/main/java/io/squashql/query/join/ExperimentalQueryJoinExecutor.java b/core/src/main/java/io/squashql/query/join/ExperimentalQueryJoinExecutor.java
index 46ed21a..4bb942d 100644
--- a/core/src/main/java/io/squashql/query/join/ExperimentalQueryJoinExecutor.java
+++ b/core/src/main/java/io/squashql/query/join/ExperimentalQueryJoinExecutor.java
@@ -19,6 +19,7 @@ import org.eclipse.collections.impl.tuple.Tuples;
 import java.util.*;
 import java.util.stream.Collectors;
 
+import static io.squashql.query.Functions.isNull;
 import static io.squashql.query.QueryExecutor.LIMIT_DEFAULT_VALUE;
 import static io.squashql.query.database.AQueryEngine.transformToColumnFormat;
 import static io.squashql.query.database.SqlTranslator.addLimit;
@@ -187,7 +188,10 @@ public class ExperimentalQueryJoinExecutor {
           for (Pair<Pair<Field, Integer>, Pair<Field, Integer>> c : common) {
             Field l = new TableField(holders.get(c.getOne().getTwo()).cteTableName, getFieldName(c.getOne().getOne()));
             Field r = new TableField(holders.get(c.getTwo().getTwo()).cteTableName, getFieldName(c.getTwo().getOne()));
-            children.add(new CriteriaDto(l, r, null, null, ConditionType.EQ, Collections.emptyList()));
+            List<CriteriaDto> nullChecks = List.of(new CriteriaDto(l, isNull()), new CriteriaDto(r, isNull()));
+            CriteriaDto nullChecksCriteriaDto = new CriteriaDto(null, null, null, null, ConditionType.AND, nullChecks);
+            CriteriaDto equalityCriteriaDto = new CriteriaDto(l, r, null, null, ConditionType.EQ, Collections.emptyList());
+            children.add(new CriteriaDto(null, null, null, null, ConditionType.OR, List.of(nullChecksCriteriaDto, equalityCriteriaDto)));
           }
           CriteriaDto criteriaDto = children.size() > 1
                   ? new CriteriaDto(null, null, null, null, ConditionType.AND, children)
@@ -211,9 +215,8 @@ public class ExperimentalQueryJoinExecutor {
    * B.id (or __cte1__.id) assuming A comes before B in the {@link Holder} list.
    */
   public String sqlExpression(CriteriaDto jc, QueryRewriter queryRewriter, List<Holder> holders, Set<String> toRemoveFromSelectSet) {
-    if (jc.field != null && jc.fieldOther != null && jc.conditionType != null) {
+    if (jc.field != null) {
       Holder leftHolder = getHolderOrigin(holders, jc.field);
-      Holder rightHolder = getHolderOrigin(holders, jc.fieldOther);
 
       String left;
       String leftFieldName = getFieldName(jc.field);
@@ -224,33 +227,41 @@ public class ExperimentalQueryJoinExecutor {
         left = SqlUtils.getFieldFullName(leftHolder == null ? null : queryRewriter.cteName(leftHolder.cteTableName), queryRewriter.fieldName(leftFieldName));
       }
 
-      String right;
-      String rightFieldName = getFieldName(jc.fieldOther);
-      if (jc.fieldOther instanceof TableField tf) {
-        right = SqlUtils.getFieldFullName(queryRewriter.cteName(tf.tableName), queryRewriter.fieldName(rightFieldName));
-      } else {
-        // could be an aliased field? Need find where it comes from
-        right = SqlUtils.getFieldFullName(rightHolder == null ? null : queryRewriter.cteName(rightHolder.cteTableName), queryRewriter.fieldName(rightFieldName));
-      }
+      if (jc.fieldOther != null && jc.conditionType != null) {
+        Holder rightHolder = getHolderOrigin(holders, jc.fieldOther);
+
+        String right;
+        String rightFieldName = getFieldName(jc.fieldOther);
+        if (jc.fieldOther instanceof TableField tf) {
+          right = SqlUtils.getFieldFullName(queryRewriter.cteName(tf.tableName), queryRewriter.fieldName(rightFieldName));
+        } else {
+          // could be an aliased field? Need find where it comes from
+          right = SqlUtils.getFieldFullName(rightHolder == null ? null : queryRewriter.cteName(rightHolder.cteTableName), queryRewriter.fieldName(rightFieldName));
+        }
 
-      String toRemoveFromSelect = null;
-      for (Holder holder : holders) {
-        if (holder.equals(leftHolder)) {
-          toRemoveFromSelect = left; // Same logic than the select i.e __cte0__.fieldName
+        String toRemoveFromSelect = null;
+        for (Holder holder : holders) {
+          if (holder.equals(leftHolder)) {
+            toRemoveFromSelect = left; // Same logic than the select i.e __cte0__.fieldName
+          }
+          if (holder.equals(rightHolder)) {
+            toRemoveFromSelect = right; // Same logic than the select i.e __cte0__.fieldName
+          }
         }
-        if (holder.equals(rightHolder)) {
-          toRemoveFromSelect = right; // Same logic than the select i.e __cte0__.fieldName
+
+        if (toRemoveFromSelect != null) {
+          toRemoveFromSelectSet.add(toRemoveFromSelect);
         }
-      }
 
-      if (toRemoveFromSelect != null) {
-        toRemoveFromSelectSet.add(toRemoveFromSelect);
+        return String.join(" ", left, jc.conditionType.sqlInfix, right);
+      } else if (jc.fieldOther == null && jc.condition != null) {
+        return String.join(" ", left, jc.condition.type().sqlInfix);
       }
-
-      return String.join(" ", left, jc.conditionType.sqlInfix, right);
+      return null;
     } else if (!jc.children.isEmpty()) {
       String sep = switch (jc.conditionType) {
         case AND -> " and ";
+        case OR -> " or ";
         default -> throw new IllegalStateException("Unexpected value: " + jc.conditionType);
       };
       Iterator<CriteriaDto> iterator = jc.children.iterator();


Test patch:
diff --git a/clickhouse/src/test/java/io/squashql/query/ClickHouseTestUtil.java b/clickhouse/src/test/java/io/squashql/query/ClickHouseTestUtil.java
index 3ac5ad7..a2a22ee 100644
--- a/clickhouse/src/test/java/io/squashql/query/ClickHouseTestUtil.java
+++ b/clickhouse/src/test/java/io/squashql/query/ClickHouseTestUtil.java
@@ -20,7 +20,7 @@ public class ClickHouseTestUtil {
           c.getMappedPort(ClickHouseProtocol.HTTP.getDefaultPort()));
 
   protected static GenericContainer createClickHouseContainer() {
-    return new GenericContainer(DockerImageName.parse("clickhouse/clickhouse-server:23.8.2.7-alpine"))
+    return new GenericContainer(DockerImageName.parse("clickhouse/clickhouse-server:24.11.1.2557-alpine"))
             .withExposedPorts(ClickHouseProtocol.HTTP.getDefaultPort(), ClickHouseProtocol.GRPC.getDefaultPort())
             .waitingFor(Wait.forHttp("/ping")
                     .forPort(ClickHouseProtocol.HTTP.getDefaultPort())
diff --git a/core/src/test/java/io/squashql/join/TestSqlExperimentalQueryJoinExecutor.java b/core/src/test/java/io/squashql/join/TestSqlExperimentalQueryJoinExecutor.java
index 2c4ca96..1db1c9f 100644
--- a/core/src/test/java/io/squashql/join/TestSqlExperimentalQueryJoinExecutor.java
+++ b/core/src/test/java/io/squashql/join/TestSqlExperimentalQueryJoinExecutor.java
@@ -206,7 +206,7 @@ public class TestSqlExperimentalQueryJoinExecutor {
             "`CTE1` as (" +
             "select `B`.`b` as `alias_b`, `B`.`d`, `B`.`e`, `B`.`f` from `B` group by `alias_b`, `B`.`d`, `B`.`e`, `B`.`f`) " +
             "select `CTE0`.`a`, `CTE0`.`alias_b` as `alias_b`, `CTE0`.`c`, `CTE1`.`d`, `CTE1`.`e`, `CTE1`.`f` " +
-            "from `CTE0` left join `CTE1` on `CTE0`.`alias_b` = `CTE1`.`alias_b` " +
+            "from `CTE0` left join `CTE1` on ((`CTE0`.`alias_b` is null and `CTE1`.`alias_b` is null) or `CTE0`.`alias_b` = `CTE1`.`alias_b`) " +
             "limit 12";
     Assertions.assertThat(result.getOne()).isEqualTo(expectedSql);
     List<String> selectElements = result.getTwo().stream().map(SqlUtils::squashqlExpression).toList();
@@ -236,7 +236,9 @@ public class TestSqlExperimentalQueryJoinExecutor {
             "`CTE1` as (" +
             "select `A`.`b`, `A`.`c` from `A` group by `A`.`b`, `A`.`c`) " +
             "select `CTE0`.`a`, `CTE0`.`b`, `CTE0`.`c` " +
-            "from `CTE0` left join `CTE1` on (`CTE0`.`b` = `CTE1`.`b` and `CTE0`.`c` = `CTE1`.`c`) " +
+            "from `CTE0` left join `CTE1` " +
+            "on (((`CTE0`.`b` is null and `CTE1`.`b` is null) or `CTE0`.`b` = `CTE1`.`b`) " +
+            "and ((`CTE0`.`c` is null and `CTE1`.`c` is null) or `CTE0`.`c` = `CTE1`.`c`)) " +
             "limit 12";
     Assertions.assertThat(result.getOne()).isEqualTo(expectedSql);
     List<String> selectElements = result.getTwo().stream().map(SqlUtils::squashqlExpression).toList();
@@ -278,7 +280,7 @@ public class TestSqlExperimentalQueryJoinExecutor {
             "select `C`.`b`, `C`.`c`, `C`.`d`, `C`.`g` from `C` group by `C`.`b`, `C`.`c`, `C`.`d`, `C`.`g`) " +
             "select `CTE0`.`a`, `CTE0`.`alias_b` as `alias_b`, `CTE0`.`c`, `CTE1`.`d`, `CTE1`.`e`, `CTE1`.`f`, `CTE2`.`g` " +
             "from `CTE0` " +
-            "left join `CTE1` on `CTE0`.`alias_b` = `CTE1`.`alias_b` " +
+            "left join `CTE1` on ((`CTE0`.`alias_b` is null and `CTE1`.`alias_b` is null) or `CTE0`.`alias_b` = `CTE1`.`alias_b`) " +
             "left join `CTE2` on (`CTE0`.`b` = `CTE2`.`b` and `CTE0`.`c` = `CTE2`.`c` and `CTE1`.`d` = `CTE2`.`d`) " +
             "limit 12";
     Assertions.assertThat(result.getOne()).isEqualTo(expectedSql);
diff --git a/core/src/test/java/io/squashql/query/ATestQueryWithJoins.java b/core/src/test/java/io/squashql/query/ATestQueryWithJoins.java
index 78f97e9..421708d 100644
--- a/core/src/test/java/io/squashql/query/ATestQueryWithJoins.java
+++ b/core/src/test/java/io/squashql/query/ATestQueryWithJoins.java
@@ -144,7 +144,7 @@ public abstract class ATestQueryWithJoins extends ABaseTestQuery {
     } else if (ds.contains(TestClass.Type.BIGQUERY.className)) {
       return "Column name name is ambiguous";
     } else if (ds.contains(TestClass.Type.CLICKHOUSE.className)) {
-      return "Ambiguous column 'name'";
+      return "ambiguous identifier 'name'";
     } else if (ds.contains(TestClass.Type.SNOWFLAKE.className)) {
       return "ambiguous column name 'name'";
     } else if (ds.contains(TestClass.Type.DUCKDB.className)) {