fewer parens

plausible · Nov 2, 2024 · ce2baeb · ce2baeb
1 parent f6c10f7
commit ce2baeb
Show file tree

Hide file tree

Showing 2 changed files with 132 additions and 17 deletions.
diff --git a/lib/ecto/adapters/clickhouse/connection.ex b/lib/ecto/adapters/clickhouse/connection.ex
@@ -298,8 +298,6 @@ defmodule Ecto.Adapters.ClickHouse.Connection do
     -: " - ",
     *: " * ",
     /: " / ",
-    and: " AND ",
-    or: " OR ",
     # TODO ilike()
     ilike: " ILIKE ",
     # TODO like()
@@ -595,17 +593,22 @@ defmodule Ecto.Adapters.ClickHouse.Connection do
 
   defp boolean(_name, [], _sources, _params, _query), do: []
 
+  defp boolean(name, [%{expr: expr}], sources, params, query) do
+    [name | expr(expr, sources, params, query)]
+  end
+
   defp boolean(name, [%{expr: expr, op: op} | exprs], sources, params, query) do
-    result =
-      Enum.reduce(exprs, {op, paren_expr(expr, sources, params, query)}, fn
+    {_last_op, result} =
+      Enum.reduce(exprs, {op, expr(expr, sources, params, query)}, fn
         %BooleanExpr{expr: expr, op: op}, {op, acc} ->
-          {op, [acc, operator_to_boolean(op) | paren_expr(expr, sources, params, query)]}
+          {op, [acc, operator_to_boolean(op) | logical_expr(op, expr, sources, params, query)]}
 
         %BooleanExpr{expr: expr, op: op}, {_, acc} ->
-          {op, [?(, acc, ?), operator_to_boolean(op) | paren_expr(expr, sources, params, query)]}
+          {op,
+           [?(, acc, ?), operator_to_boolean(op) | logical_expr(op, expr, sources, params, query)]}
       end)
 
-    [name | elem(result, 1)]
+    [name | result]
   end
 
   defp operator_to_boolean(:and), do: " AND "
@@ -798,6 +801,14 @@ defmodule Ecto.Adapters.ClickHouse.Connection do
     ["exists" | expr(subquery, sources, params, query)]
   end
 
+  defp expr({op, _, [l, r]}, sources, params, query) when op in [:and, :or] do
+    [
+      logical_expr(op, l, sources, params, query),
+      operator_to_boolean(op),
+      logical_expr(op, r, sources, params, query)
+    ]
+  end
+
   defp expr({fun, _, args}, sources, params, query) when is_atom(fun) and is_list(args) do
     case handle_call(fun, length(args)) do
       {:binary_op, op} ->
@@ -852,6 +863,29 @@ defmodule Ecto.Adapters.ClickHouse.Connection do
       message: "unsupported expression #{inspect(expr)}"
   end
 
+  defp logical_expr(parent_op, expr, sources, params, query) do
+    case expr do
+      {^parent_op, _, [l, r]} ->
+        [
+          logical_expr(parent_op, l, sources, params, query),
+          operator_to_boolean(parent_op),
+          logical_expr(parent_op, r, sources, params, query)
+        ]
+
+      {op, _, [l, r]} when op in [:and, :or] ->
+        [
+          ?(,
+          logical_expr(op, l, sources, params, query),
+          operator_to_boolean(op),
+          logical_expr(op, r, sources, params, query),
+          ?)
+        ]
+
+      _ ->
+        maybe_paren_expr(expr, sources, params, query)
+    end
+  end
+
   defp maybe_paren_expr({op, _, [_, _]} = expr, sources, params, query) when op in @binary_ops do
     paren_expr(expr, sources, params, query)
   end

diff --git a/test/ecto/adapters/clickhouse/connection_test.exs b/test/ecto/adapters/clickhouse/connection_test.exs
@@ -395,24 +395,105 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
     assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0 WHERE ((s0."x",s0."y") > (1,2))]
   end
 
-  test "where with big AND chain gets many parens" do
-    assert all(from e in "events", where: true and true and true and true and true, select: true) ==
+  test "single where with AND/OR chains" do
+    assert all(
+             from e in "events",
+               where: true and true and true and true and true,
+               select: true
+           ) ==
+             """
+             SELECT true FROM "events" AS e0 WHERE 1 AND 1 AND 1 AND 1 AND 1\
+             """
+
+    assert all(
+             from e in "events",
+               where: true or true or true or true or true,
+               select: true
+           ) ==
+             """
+             SELECT true FROM "events" AS e0 WHERE 1 OR 1 OR 1 OR 1 OR 1\
+             """
+
+    # :) explain syntax select 1 AND 1 AND 1 OR 1 OR 1 OR 1 AND 1 OR 1;
+    # SELECT (1 AND 1 AND 1) OR 1 OR 1 OR (1 AND 1) OR 1
+    assert all(
+             from e in "events",
+               where: (true and true and true) or true or true or (true and true) or true,
+               select: true
+           ) ==
+             """
+             SELECT true FROM "events" AS e0 WHERE (1 AND 1 AND 1) OR 1 OR 1 OR (1 AND 1) OR 1\
+             """
+
+    assert all(
+             from e in "events",
+               where: (true and true and true) or true or true or (true and (true or true)),
+               select: true
+           ) ==
              """
-             SELECT true FROM "events" AS e0 WHERE ((((1 AND 1) AND 1) AND 1) AND 1)\
+             SELECT true FROM "events" AS e0 WHERE (1 AND 1 AND 1) OR 1 OR 1 OR (1 AND (1 OR 1))\
              """
   end
 
-  test "many where clauses get flat AND chain" do
+  test "many where with AND/OR chains" do
+    assert all(
+             from e in "events",
+               where: fragment("1"),
+               where: fragment("2"),
+               where: fragment("3"),
+               where: fragment("4"),
+               where: fragment("5"),
+               select: true
+           ) == """
+           SELECT true FROM "events" AS e0 WHERE 1 AND 2 AND 3 AND 4 AND 5\
+           """
+
+    assert all(
+             from e in "events",
+               or_where: fragment("1"),
+               or_where: fragment("2"),
+               or_where: fragment("3"),
+               or_where: fragment("4"),
+               or_where: fragment("5"),
+               select: true
+           ) == """
+           SELECT true FROM "events" AS e0 WHERE 1 OR 2 OR 3 OR 4 OR 5\
+           """
+
+    # NOTE: when multiple :where/:or_where are used, the order is preserved using parens with left-to-right precedence
+    assert all(
+             from e in "events",
+               where: fragment("1"),
+               where: fragment("2"),
+               where: fragment("3"),
+               or_where: fragment("4"),
+               or_where: fragment("5"),
+               where: fragment("6"),
+               where: fragment("7"),
+               or_where: fragment("8"),
+               select: true
+           ) == """
+           SELECT true FROM "events" AS e0 WHERE (((1 AND 2 AND 3) OR 4 OR 5) AND 6 AND 7) OR 8\
+           """
+  end
+
+  test "many where with AND/OR chains inside" do
+    assert all(
+             from e in "events",
+               where: true and true and true,
+               where: true and true,
+               select: true
+           ) == """
+           SELECT true FROM "events" AS e0 WHERE 1 AND 1 AND 1 AND 1 AND 1\
+           """
+
     assert all(
              from e in "events",
-               where: true,
-               where: true,
-               where: true,
-               where: true,
-               where: true,
+               where: true and true and true,
+               or_where: true and true,
                select: true
            ) == """
-           SELECT true FROM "events" AS e0 WHERE (1) AND (1) AND (1) AND (1) AND (1)\
+           SELECT true FROM "events" AS e0 WHERE (1 AND 1 AND 1) OR (1 AND 1)\
            """
   end