From 70bbb4c0cf61a69c517eeeeb3076c6d4382c793e Mon Sep 17 00:00:00 2001 From: ruslandoga Date: Tue, 5 Nov 2024 14:19:59 +0700 Subject: [PATCH] fewer parens (#207) --- CHANGELOG.md | 4 + lib/ecto/adapters/clickhouse/connection.ex | 33 ++++- .../adapters/clickhouse/connection_test.exs | 119 +++++++++++++++++- 3 files changed, 149 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19332e9..88b8467 100644 --- 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 index ca5df17..b513c81 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() @@ -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 index 45f6f8a..2efa567 100644 --- a/test/ecto/adapters/clickhouse/connection_test.exs +++ b/test/ecto/adapters/clickhouse/connection_test.exs @@ -395,13 +395,15 @@ 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 + # TODO remove (kept for smaller diffs) test "many where clauses get flat AND chain" do assert all( from e in "events", @@ -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,9 +564,10 @@ 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")) @@ -466,7 +575,7 @@ 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 @@ -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])