From 4833f6b3afc7483292711b382abe448f00aed830 Mon Sep 17 00:00:00 2001 From: ruslandoga <67764432+ruslandoga@users.noreply.github.com> Date: Sat, 2 Nov 2024 21:37:59 +0700 Subject: [PATCH] fewer parens --- CHANGELOG.md | 4 + lib/ecto/adapters/clickhouse/connection.ex | 48 +++++- .../adapters/clickhouse/connection_test.exs | 141 ++++++++++++++---- test/ecto/integration/inline_test.exs | 13 +- test/ecto_ch_test.exs | 2 +- 5 files changed, 163 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19332e9e..c07d5a7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- remove 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 index ca5df17d..fc3e4780 100644 --- 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 | maybe_paren_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, maybe_paren_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 index 45f6f8a9..71d7f2cb 100644 --- a/test/ecto/adapters/clickhouse/connection_test.exs +++ b/test/ecto/adapters/clickhouse/connection_test.exs @@ -223,7 +223,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do assert all(query) == """ WITH RECURSIVE "tree" AS \ - (SELECT sc0."id" AS "id",1 AS "depth" FROM "categories" AS sc0 WHERE (isNull(sc0."parent_id")) \ + (SELECT sc0."id" AS "id",1 AS "depth" FROM "categories" AS sc0 WHERE isNull(sc0."parent_id") \ UNION ALL \ (SELECT c0."id",t1."depth" + 1 FROM "categories" AS c0 \ INNER JOIN "tree" AS t1 ON t1."id" = c0."parent_id")) \ @@ -258,7 +258,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do """ WITH "comments_scope" AS (\ SELECT sc0."entity_id" AS "entity_id",sc0."text" AS "text" \ - FROM "comments" AS sc0 WHERE (isNull(sc0."deleted_at"))) \ + FROM "comments" AS sc0 WHERE isNull(sc0."deleted_at")) \ SELECT p0."title",c1."text" \ FROM "posts" AS p0 \ INNER JOIN "comments_scope" AS c1 ON c1."entity_id" = p0."guid" \ @@ -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 @@ -449,16 +530,17 @@ 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}))\ + SELECT count(*) FROM "events" AS e0 WHERE (e0."site_id" = {$0:Int64}) AND (e0."timestamp" < {$1:Date})\ """ dynamic = dynamic([e], ^dynamic and e.timestamp >= ^~D[2024-01-01]) 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")) @@ -466,7 +548,11 @@ 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})) 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 @@ -750,7 +836,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> select([r], r.x) |> where([], fragment(~s|? = "query\\?"|, ^10)) - assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0 WHERE ({$0:Int64} = "query?")] + assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0 WHERE {$0:Int64} = "query?"] value = 13 query = Schema |> select([r], fragment("lcase(?, ?)", r.x, ^value)) @@ -791,7 +877,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> where(fragment("? = ?", literal(^name), "Main")) |> select([], true) - assert all(query) == ~s|SELECT true FROM "schema" AS s0 WHERE ("y" = 'Main')| + assert all(query) == ~s|SELECT true FROM "schema" AS s0 WHERE "y" = 'Main'| end test "selected_as" do @@ -882,7 +968,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 +1128,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}\ @@ -1051,18 +1136,16 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do SELECT s0."id",{$3:Int64} FROM "schema" AS s0 \ INNER JOIN "schema2" AS s1 ON {$4:Bool} \ INNER JOIN "schema2" AS s2 ON {$5:Bool} \ - WHERE ({$6:Bool}) AND ({$7:Bool}) \ + WHERE {$6:Bool} AND {$7:Bool} \ GROUP BY {$8:Int64},{$9:Int64} \ - HAVING ({$10:Bool}) AND ({$11:Bool}) \ + HAVING {$10:Bool} AND {$11:Bool} \ ORDER BY {$16:Int64} \ 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 @@ -1074,7 +1157,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do select: true ) - assert all(query) == ~s|SELECT true FROM "schema" AS s0 WHERE (s0."start_time" = "query?")| + assert all(query) == ~s|SELECT true FROM "schema" AS s0 WHERE s0."start_time" = "query?"| end test "update_all" do @@ -1716,7 +1799,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do FROM schema2 AS s2 \ WHERE s2.id = s0."x" AND s2.field = {$1:Int64}\ ) AS f1 ON 1 \ - WHERE ((s0."id" > 0) AND (s0."id" < {$2:Int64}))\ + WHERE (s0."id" > 0) AND (s0."id" < {$2:Int64})\ """ end @@ -2143,7 +2226,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do insert = insert(nil, "schema", [:foo, :bar], select, {:raise, [], []}, []) assert insert == - ~s{INSERT INTO "schema"("foo","bar") SELECT 3,s0."bar" FROM "schema" AS s0 WHERE (1)} + ~s{INSERT INTO "schema"("foo","bar") SELECT 3,s0."bar" FROM "schema" AS s0 WHERE 1} select = (s in "schema") diff --git a/test/ecto/integration/inline_test.exs b/test/ecto/integration/inline_test.exs index bf0774fb..57a6f8f3 100644 --- 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\ @@ -62,18 +61,16 @@ defmodule Ecto.Integration.InlineSQLTest do SELECT s0."id",0 FROM "schema" AS s0 \ INNER JOIN "schema2" AS s1 ON true \ INNER JOIN "schema2" AS s2 ON false \ - WHERE (true) AND (false) \ + WHERE true AND false \ GROUP BY 3,4 \ - HAVING (true) AND (false) \ + HAVING true AND false \ ORDER BY 7 \ 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 diff --git a/test/ecto_ch_test.exs b/test/ecto_ch_test.exs index 8c0ac7bf..7917a566 100644 --- a/test/ecto_ch_test.exs +++ b/test/ecto_ch_test.exs @@ -48,7 +48,7 @@ defmodule EctoCh.Test do |> select([e], e.user_id) assert all(query) == - {~s[SELECT e0."user_id" FROM "events" AS e0 WHERE (name = {$0:String})], ["John"]} + {~s[SELECT e0."user_id" FROM "events" AS e0 WHERE name = {$0:String}], ["John"]} end test "where in" do