fewer parens

plausible · Nov 4, 2024 · 778de16 · 778de16
1 parent ceabebf
commit 778de16
Show file tree

Hide file tree

Showing 4 changed files with 158 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- remove some unnecessary parens from generated SQL to avoid hitting TOO_DEEP_RECURSION https://github.com/plausible/ecto_ch/pull/207
+
 ## 0.4.0 (2024-10-15)
 
 - use `UNION DISTINCT` instead of `UNION` for `Ecto.Query.union/2` https://github.com/plausible/ecto_ch/pull/204

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()
@@ -798,6 +796,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 +858,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,14 +395,16 @@ 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
+  # TODO remove (kept for smaller diffs)
+  test "where with big AND chain (NOT) gets many parens" 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)\
+             SELECT true FROM "events" AS e0 WHERE (1 AND 1 AND 1 AND 1 AND 1)\
              """
   end
 
-  test "many where clauses get flat AND chain" do
+  # TODO remove (kept for smaller diffs)
+  test "many where clauses get flat chain" do
     assert all(
              from e in "events",
                where: true,
@@ -416,6 +418,112 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
            """
   end
 
+  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) OR 1 OR 1 OR (1 AND (1 OR 1))\
+             )\
+             """
+  end
+
+  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 and true and true,
+               or_where: true and true,
+               select: true
+           ) == """
+           SELECT true FROM "events" AS e0 WHERE ((1 AND 1 AND 1)) OR (1 AND 1)\
+           """
+  end
+
   test "or_where" do
     query =
       Schema
@@ -456,17 +564,18 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
 
     assert all(from e in "events", where: ^dynamic, select: count()) ==
              """
-             SELECT count(*) FROM "events" AS e0 WHERE (((e0."site_id" = {$0:Int64}) AND (e0."timestamp" < {$1:Date})) AND (e0."timestamp" >= {$2:Date}))\
+             SELECT count(*) FROM "events" AS e0 WHERE ((e0."site_id" = {$0:Int64}) AND (e0."timestamp" < {$1:Date}) AND (e0."timestamp" >= {$2:Date}))\
              """
 
+    # imagine:
     # (has_key(t, :"meta.key", ^"content-platform") and
     #    get_by_key(t, :"meta.key", ^"content-platform") in ^["(none)", "web", "app"]) or
     #   (^true and not has_key(t, :"meta.key", ^"content-platform"))
     dynamic = dynamic([e], ^dynamic and ((true and false) or (true and not true)))
 
     assert all(from e in "events", where: ^dynamic, select: count()) ==
              """
-             SELECT count(*) FROM "events" AS e0 WHERE ((((e0."site_id" = {$0:Int64}) AND (e0."timestamp" < {$1:Date})) AND (e0."timestamp" >= {$2:Date})) AND ((1 AND 0) OR (1 AND not(1))))\
+             SELECT count(*) FROM "events" AS e0 WHERE ((e0."site_id" = {$0:Int64}) AND (e0."timestamp" < {$1:Date}) AND (e0."timestamp" >= {$2:Date}) AND ((1 AND 0) OR (1 AND not(1))))\
              """
   end
 
@@ -882,7 +991,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
     query = Schema |> select([e], e.x == ^0 or e.x in ^[1, 2, 3] or e.x == ^4)
 
     assert all(query) ==
-             ~s[SELECT ((s0."x" = {$0:Int64}) OR (s0."x" IN ({$1:Int64},{$2:Int64},{$3:Int64}))) OR (s0."x" = {$4:Int64}) FROM "schema" AS s0]
+             ~s[SELECT (s0."x" = {$0:Int64}) OR (s0."x" IN ({$1:Int64},{$2:Int64},{$3:Int64})) OR (s0."x" = {$4:Int64}) FROM "schema" AS s0]
 
     query = Schema |> select([e], e in [1, 2, 3])
 
@@ -1042,8 +1151,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
              """
              WITH \
              "cte1" AS (\
-             SELECT ss0."id" AS "id",{$0:Bool} AS "smth" FROM "schema1" AS ss0 \
-             WHERE ({$1:Int64})\
+             SELECT ss0."id" AS "id",{$0:Bool} AS "smth" FROM "schema1" AS ss0 WHERE ({$1:Int64})\
              ),\
              "cte2" AS (\
              SELECT * FROM schema WHERE {$2:Int64}\
@@ -1058,11 +1166,9 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
              LIMIT {$17:Int64} \
              OFFSET {$18:Int64} \
              UNION DISTINCT \
-             (SELECT s0."id",{$12:Bool} FROM "schema1" AS s0 \
-             WHERE ({$13:Int64})) \
+             (SELECT s0."id",{$12:Bool} FROM "schema1" AS s0 WHERE ({$13:Int64})) \
              UNION ALL \
-             (SELECT s0."id",{$14:Bool} FROM "schema2" AS s0 \
-             WHERE ({$15:Int64}))\
+             (SELECT s0."id",{$14:Bool} FROM "schema2" AS s0 WHERE ({$15:Int64}))\
              """
   end
 
@@ -1695,7 +1801,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
         :inner,
         [p],
         q in fragment(
-          "SELECT * FROM schema2 AS s2 WHERE s2.id = ? AND s2.field = ?",
+          "SELECT * FROM schema2 AS s2 WHERE (s2.id = ? AND s2.field = ?)",
           p.x,
           ^10
         ),
@@ -1714,7 +1820,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
              (\
              SELECT * \
              FROM schema2 AS s2 \
-             WHERE s2.id = s0."x" AND s2.field = {$1:Int64}\
+             WHERE (s2.id = s0."x" AND s2.field = {$1:Int64})\
              ) AS f1 ON 1 \
              WHERE ((s0."id" > 0) AND (s0."id" < {$2:Int64}))\
              """

diff --git a/test/ecto/integration/inline_test.exs b/test/ecto/integration/inline_test.exs
@@ -53,8 +53,7 @@ defmodule Ecto.Integration.InlineSQLTest do
                """
                WITH \
                "cte1" AS (\
-               SELECT ss0."id" AS "id",true AS "smth" FROM "schema1" AS ss0 \
-               WHERE (1)\
+               SELECT ss0."id" AS "id",true AS "smth" FROM "schema1" AS ss0 WHERE (1)\
                ),\
                "cte2" AS (\
                SELECT * FROM schema WHERE 2\
@@ -69,11 +68,9 @@ defmodule Ecto.Integration.InlineSQLTest do
                LIMIT 8 \
                OFFSET 9 \
                UNION DISTINCT \
-               (SELECT s0."id",true FROM "schema1" AS s0 \
-               WHERE (5)) \
+               (SELECT s0."id",true FROM "schema1" AS s0 WHERE (5)) \
                UNION ALL \
-               (SELECT s0."id",false FROM "schema2" AS s0 \
-               WHERE (6))\
+               (SELECT s0."id",false FROM "schema2" AS s0 WHERE (6))\
                """
     end