Repository: elixir-ecto/ecto
Pull request #2487 (2018-03-30 17:45:52)

Support for using FILTER with aggregates
I haven't added any tests or documentation yet, since I figured there might still be some discussion about this. I also kinda winged it on the path from select expression -> builder -> SQL. I'm not sure how far away from the right answer I am there. This works for me in my manual testing, and if it doesn't look too far off, I can get started writing actual tests for it.

This was discussed here: https://groups.google.com/forum/#!topic/elixir-ecto/9APrq9XfDp0

josevalim: I think this is almost all you need to do. There is a type-based concern I will comment on soon. For the tests, adding simple cases to test/ecto/adapters/postgres and checking that mysql raises for similar construct is enough.
josevalim: You will also need to add a clause around here that handles filters:

https://github.com/elixir-ecto/ecto/blob/8d5fd3857ab16bb0521d172044ec1f343900a347/lib/ecto/query/planner.ex#L954

You can probably find a bug that will show the need for such clause by adding some tests to integration_test/cases/types.exs that uses the new filter. The test will show that Ecto is not properly casting the result.

That's because we need to teach Ecto that only the first argument of filter matters for the type computation.

zachdaniel: @josevalim I will add that to the planner :) I'll see if I can write some failing tests w/o it first, so I can be confident that I covered that behavior.
zachdaniel: Test failed, like you said :)
ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 possible new issue (including those that may have been commented here).

But beware that this branch is **[2 commits behind the `elixir-ecto:master` branch](https://github.com/elixir-ecto/ecto/compare/zachdaniel:filter-aggregates...elixir-ecto:master)**, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2487.

<!-- Ebert auto generated comment -->

zachdaniel: @josevalim I feel like just the documentation in `Query.Api` isn't enough, but I'm not sure where else to put it. Any thoughts?
zachdaniel: @josevalim interesting. I didn't realize it, but support for this feature was only added in postgres 9.4. Is there some way to annotate this in the tests, or does that rule out this feature?
zachdaniel: Found the spot where I can configure it in test_helper.ex
zachdaniel: Alright, ready to go!
zachdaniel: Alright, added custom error messaging in the mysql adapter for the error message, simplified the implementation, and removed the varargs aspect of `filter`, so the examples just use `and` for multiple conditions. I totally agree the varargs didn't make much sense, I think I was just learning how to implement it, and thought to myself "what if there are more things in the list of arguments", and then the shape of that just kinda snuck into my work.
zachdaniel: @josevalim @michalmuskala Responded to your feedback.
zachdaniel: Not sure whats going on with ebert. Could that be related to my changes at all?

Patch:
diff --git a/lib/ecto/adapters/mysql/connection.ex b/lib/ecto/adapters/mysql/connection.ex
index 359319b7..b778788c 100644
--- a/lib/ecto/adapters/mysql/connection.ex
+++ b/lib/ecto/adapters/mysql/connection.ex
@@ -411,6 +411,10 @@ if Code.ensure_loaded?(Mariaex) do
       ["NOT (", expr(expr, sources, query), ?)]
     end
 
+    defp expr({:filter, _, _}, _sources, query) do
+      error!(query, "MySQL adapter does not support aggregate filters")
+    end
+
     defp expr(%Ecto.SubQuery{query: query}, _sources, _query) do
       all(query)
     end
diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex
index 81fafa8a..7b889af6 100644
--- a/lib/ecto/adapters/postgres/connection.ex
+++ b/lib/ecto/adapters/postgres/connection.ex
@@ -482,6 +482,11 @@ if Code.ensure_loaded?(Postgrex) do
        interval(count, interval, sources, query) | ")::date"]
     end
 
+    defp expr({:filter, _, [agg, filter]}, sources, query) do
+      aggregate = expr(agg, sources, query)
+      [aggregate, " FILTER (WHERE ", expr(filter, sources, query), ?)]
+    end
+
     defp expr({:{}, _, elems}, sources, query) do
       [?(, intersperse_map(elems, ?,, &expr(&1, sources, query)), ?)]
     end
diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex
index cf49a92f..be24abb7 100644
--- a/lib/ecto/query/api.ex
+++ b/lib/ecto/query/api.ex
@@ -158,6 +158,16 @@ defmodule Ecto.Query.API do
   """
   def count(value, :distinct), do: doc! [value, :distinct]
 
+  @doc """
+  Applies the given expression as a FILTER clause against an
+  aggregate. This is currently only supported by Postgres.
+
+      from p in Payment, select: filter(avg(p.value), p.value > 0 and p.value < 100)
+
+      from p in Payment, select: avg(p.value) |> filter(p.value < 0)
+  """
+  def filter(value, filter), do: doc! [value, filter]
+
   @doc """
   Calculates the average for the given entry.
 
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index a0676a9f..66461190 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -248,6 +248,15 @@ defmodule Ecto.Query.Builder do
     {expr, params_acc}
   end
 
+  def escape({:filter, _, [aggregate]}, type, params_acc, vars, env) do
+    escape(aggregate, type, params_acc, vars, env)
+  end
+  def escape({:filter, _, [aggregate, filter_expr]}, type, params_acc, vars, env) do
+    {aggregate, params_acc} = escape(aggregate, type, params_acc, vars, env)
+    {filter_expr, params_acc} = escape(filter_expr, type, params_acc, vars, env)
+    {{:{}, [], [:filter, [], [aggregate, filter_expr]]}, params_acc}
+  end
+
   def escape({:=, _, _} = expr, _type, _params_acc, _vars, _env) do
     error! "`#{Macro.to_string(expr)}` is not a valid query expression. " <>
             "The match operator is not supported: `=`. " <>
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 7f499a9e..997ba271 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -952,6 +952,11 @@ defmodule Ecto.Query.Planner do
     {{:value, type}, [expr | fields], from}
   end
 
+  defp collect_fields({:filter, _, [call, _]} = expr, fields, from, query, take) do
+    {type, _, _} = collect_fields(call, fields, from, query, take)
+    {type, [expr | fields], from}
+  end
+
   defp collect_fields({{:., _, [{:&, _, [ix]}, field]}, _, []} = expr,
                       fields, from, %{select: select} = query, _take) do
     type = source_type!(:select, query, select, ix, field)


Test patch:
diff --git a/integration_test/cases/type.exs b/integration_test/cases/type.exs
index 2d0deaf9..d3edd3c2 100644
--- a/integration_test/cases/type.exs
+++ b/integration_test/cases/type.exs
@@ -61,6 +61,14 @@ defmodule Ecto.Integration.TypeTest do
     assert [^datetime] = TestRepo.all(query)
   end
 
+  @tag :aggregate_filters
+  test "aggregate filter types" do
+    datetime = ~N[2014-01-16 20:26:51]
+    TestRepo.insert!(%Post{inserted_at: datetime})
+    query = from p in Post, select: filter(max(p.inserted_at), p.public == ^true)
+    assert [^datetime] = TestRepo.all(query)
+  end
+
   test "tagged types" do
     TestRepo.insert!(%Post{})
 
diff --git a/integration_test/mysql/test_helper.exs b/integration_test/mysql/test_helper.exs
index 8a07f277..ae714ade 100644
--- a/integration_test/mysql/test_helper.exs
+++ b/integration_test/mysql/test_helper.exs
@@ -3,7 +3,7 @@ Logger.configure(level: :info)
 # :modify_column is supported on MySQL 5.7
 # but that is not yet supported in travis.
 ExUnit.start exclude: [:array_type, :read_after_writes, :returning, :modify_column,
-                       :strict_savepoint, :create_index_if_not_exists,
+                       :strict_savepoint, :create_index_if_not_exists, :aggregate_filters,
                        :transaction_isolation, :rename_column, :with_conflict_target]
 
 # Configure Ecto for support and tests
diff --git a/integration_test/pg/test_helper.exs b/integration_test/pg/test_helper.exs
index 8de67d4a..1f0d7e6c 100644
--- a/integration_test/pg/test_helper.exs
+++ b/integration_test/pg/test_helper.exs
@@ -86,7 +86,7 @@ if Version.match?(version, "~> 9.5") do
   ExUnit.configure(exclude: [:without_conflict_target])
 else
   Application.put_env(:ecto, :postgres_map_type, "json")
-  ExUnit.configure(exclude: [:upsert, :upsert_all, :array_type])
+  ExUnit.configure(exclude: [:upsert, :upsert_all, :array_type, :aggregate_filters])
 end
 
 :ok = Ecto.Migrator.up(TestRepo, 0, Ecto.Integration.Migration, log: false)
diff --git a/test/ecto/adapters/mysql_test.exs b/test/ecto/adapters/mysql_test.exs
index 957f683b..ee95c79a 100644
--- a/test/ecto/adapters/mysql_test.exs
+++ b/test/ecto/adapters/mysql_test.exs
@@ -166,6 +166,13 @@ defmodule Ecto.Adapters.MySQLTest do
     assert all(query) == ~s{SELECT TRUE FROM `schema` AS s0 LIMIT 3 OFFSET 5}
   end
 
+  test "aggregate filters" do
+    query = Schema |> select([r], count(r.x) |> filter(r.x > 10)) |> normalize
+    assert_raise Ecto.QueryError, ~r/MySQL adapter does not support aggregate filters in query/, fn ->
+      all(query)
+    end
+  end
+
   test "lock" do
     query = Schema |> lock("LOCK IN SHARE MODE") |> select([], true) |> normalize
     assert all(query) == ~s{SELECT TRUE FROM `schema` AS s0 LOCK IN SHARE MODE}
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index 5685d231..68157c7c 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -116,6 +116,14 @@ defmodule Ecto.Adapters.PostgresTest do
     assert all(query) == ~s{SELECT count(DISTINCT s0."x") FROM "schema" AS s0}
   end
 
+  test "aggregate filters" do
+    query = Schema |> select([r], count(r.x) |> filter(r.x > 10)) |> normalize
+    assert all(query) == ~s{SELECT count(s0."x") FILTER (WHERE s0."x" > 10) FROM "schema" AS s0}
+
+    query = Schema |> select([r], count(r.x) |> filter(r.x > 10 and r.x < 50)) |> normalize
+    assert all(query) == ~s{SELECT count(s0."x") FILTER (WHERE (s0."x" > 10) AND (s0."x" < 50)) FROM "schema" AS s0}
+  end
+
   test "distinct" do
     query = Schema |> distinct([r], r.x) |> select([r], {r.x, r.y}) |> normalize
     assert all(query) == ~s{SELECT DISTINCT ON (s0."x") s0."x", s0."y" FROM "schema" AS s0}