Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically set first order_by when distinct set #504

Merged
merged 2 commits into from
Mar 8, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions lib/ecto/adapters/postgres/connection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,17 @@ if Code.ensure_loaded?(Postgrex.Connection) do
alias Ecto.Query.JoinExpr

def all(query) do
sources = create_names(query)
sources = create_names(query)
distinct = query.distinct
distinct_exprs = distinct_exprs(distinct, sources)

from = from(sources)
select = select(query.select, query.distinct, sources)
select = select(query.select, distinct, distinct_exprs, sources)
join = join(query.joins, sources)
where = where(query.wheres, sources)
group_by = group_by(query.group_bys, sources)
having = having(query.havings, sources)
order_by = order_by(query.order_bys, sources)
order_by = order_by(query.order_bys, distinct, distinct_exprs, sources)
limit = limit(query.limit, sources)
offset = offset(query.offset, sources)
lock = lock(query.lock)
Expand Down Expand Up @@ -160,18 +162,21 @@ if Code.ensure_loaded?(Postgrex.Connection) do

defp handle_call(fun, _arity), do: {:fun, Atom.to_string(fun)}

defp select(%SelectExpr{fields: fields}, distinct, sources) do
"SELECT " <>
distinct(distinct, sources) <>
defp select(%SelectExpr{fields: fields}, distinct, distinct_exprs, sources) do
"SELECT " <>
distinct(distinct, distinct_exprs) <>
Enum.map_join(fields, ", ", &expr(&1, sources))
end

defp distinct(nil, _sources), do: ""
defp distinct(%QueryExpr{expr: true}, _sources), do: "DISTINCT "
defp distinct(%QueryExpr{expr: false}, _sources), do: ""
defp distinct(%QueryExpr{expr: exprs}, sources) when is_list(exprs) do
"DISTINCT ON (" <> Enum.map_join(exprs, ", ", &expr(&1, sources)) <> ") "
defp distinct_exprs(%QueryExpr{expr: exprs}, sources) when is_list(exprs) do
Enum.map_join(exprs, ", ", &expr(&1, sources))
end
defp distinct_exprs(_, _), do: ""

defp distinct(nil, _sources), do: ""
defp distinct(%QueryExpr{expr: true}, _exprs), do: "DISTINCT "
defp distinct(%QueryExpr{expr: false}, _exprs), do: ""
defp distinct(_query, exprs), do: "DISTINCT ON (" <> exprs <> ") "

defp from(sources) do
{table, name, _model} = elem(sources, 0)
Expand Down Expand Up @@ -248,16 +253,21 @@ if Code.ensure_loaded?(Postgrex.Connection) do
end
end

defp order_by(order_bys, sources) do
defp order_by(order_bys, distinct, distinct_exprs, sources) do
exprs =
Enum.map_join(order_bys, ", ", fn
%QueryExpr{expr: expr} ->
Enum.map_join(expr, ", ", &order_by_expr(&1, sources))
end)

case exprs do
"" -> nil
_ -> "ORDER BY " <> exprs
case {distinct_exprs, exprs, distinct} do
{_, "", _} -> nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the distinct_exprs to order by even if there is no order_by, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericmj said no. :)

{"", _, _} ->
"ORDER BY " <> exprs
{_, _, %QueryExpr{expr: [{:&, _, _}]}} ->
"ORDER BY " <> exprs
{_, _, _} ->
"ORDER BY " <> distinct_exprs <> ", " <> exprs
end
end

Expand Down
11 changes: 6 additions & 5 deletions lib/ecto/query.ex
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,11 @@ defmodule Ecto.Query do
Only keep one row for each combination of values in the `distinct` query
expression.

The row that is being kept depends on the ordering of the rows. To ensure
results are consistent, if an `order_by` expression is also added to the
query, its leftmost part must first reference all the fields in the
`distinct` expression before referencing another field.
The row that is being kept depends on the ordering of the rows. When an
`order_by` expression is also added to the query, all fields in the `distinct`
expression are automatically referenced in the compiled SQL query.
That way, you only need to specify the fields you want to order on to
the `order_by` expression.

## Keywords examples

Expand All @@ -420,7 +421,7 @@ defmodule Ecto.Query do
# expressions to distinct too
from(p in Post,
distinct: p.category,
order_by: [p.category, p.date])
order_by: [p.date])

## Expressions examples

Expand Down
6 changes: 6 additions & 0 deletions test/ecto/adapters/postgres_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ defmodule Ecto.Adapters.PostgresTest do
query = Model |> order_by([r], [asc: r.x, desc: r.y]) |> select([r], r.x) |> normalize
assert SQL.all(query) == ~s{SELECT m0."x" FROM "model" AS m0 ORDER BY m0."x", m0."y" DESC}

query = Model |> order_by([r], [r.y]) |> distinct([r], r.x) |> select([r], r.x) |> normalize
assert SQL.all(query) == ~s{SELECT DISTINCT ON (m0."x") m0."x" FROM "model" AS m0 ORDER BY m0."x", m0."y"}

query = Model |> order_by([r], [r.y]) |> distinct([r], r) |> select([r], r) |> normalize
assert SQL.all(query) == ~s{SELECT DISTINCT ON (m0."id", m0."x", m0."y") m0."id", m0."x", m0."y" FROM "model" AS m0 ORDER BY m0."y"}

query = Model |> order_by([r], []) |> select([r], r.x) |> normalize
assert SQL.all(query) == ~s{SELECT m0."x" FROM "model" AS m0}
end
Expand Down