for SELECT DISTINCT, ORDER BY expressions must appear in select list

[dvv]

Elixir version

18.2

Database and Version

PostgreSQL 17.2

Ecto Versions

3.12.5

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.19.3

Current behavior

defmodule Foo do
  use Ecto.Schema
  schema "foo" do
    has_one :bar, Bar
  end
end

defmodule Bar do
  use Ecto.Schema
  schema "bar" do
    field :name, :string
  end
end

Repo.all(from foo in Foo, join: bar in assoc(foo, :bar), order_by: [asc: bar.name])
# SELECT f0.id FROM foo AS f0 INNER JOIN bar AS b1 ON b1.foo_id = f0.id ORDER BY b1.name
Repo.all(from foo in Foo, join: bar in assoc(foo, :bar), order_by: [asc: bar.name], distinct: true)
# SELECT DISTINCT f0.id FROM foo AS f0 INNER JOIN bar AS b1 ON b1.foo_id = f0.id ORDER BY b1.name
# ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list

Expected behavior

On distinct: true order_by-ed fields to auto-merge select list:

Repo.all(from foo in Foo, join: bar in assoc(foo, :bar), order_by: [asc: bar.name], distinct: true)
# SELECT DISTINCT f0.id, b1.name FROM foo AS f0 INNER JOIN bar AS b1 ON b1.foo_id = f0.id ORDER BY b1.name

Notice that distinct: [asc: bar.name] (which otherwise would do) won't go here.
Notice that one can not select_merge order_by-ed fields manually since they don't belong to the struct.

[dvv]

I prepended https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/postgres/connection.ex#L536 with

      # NB: append order_by fields if distinct: true specified
      fields = case query.distinct do
        %Ecto.Query.ByExpr{expr: true} -> fields ++ Enum.map(query.order_bys, & Keyword.values(&1.expr)) |> List.flatten()
        _ -> fields
      end

as a temporary workaround.

[josevalim]

Please do submit a pull request. Updating the unit tests in ecto_sql should be enough!

[dvv]

Wonder if it is the right place to do things?
I was unable to find another point at which I have %{select: fields} filled up.

[dvv]

@josevalim couldn't we change Ecto.Repo.Queryable process function so that it would allow for |> select_merge([x], %{false: ...}) act as no-op:

  # Ecto.Repo.Queryable
  defp process(row, {:merge, left, right}, from, adapter) do
     ...
        {%{__struct__: name}, %{}} ->
          for {key, _} <- right, key != false and not Map.has_key?(left, key) do
            raise ArgumentError, "struct #{inspect(name)} does not have the key #{inspect(key)}"
          end
          Map.merge(left, right |> Map.drop([false]))
     ...

This would allow to control select clause without changing output format and solve the issue.

[v0idpwn]

Ignore me if known/not useful, but you can:

Repo.all(from foo in Foo, join: bar in assoc(foo, :bar), order_by: [asc: bar.name],  select: {foo, bar.name}, distinct: true)

[greg-rychlewski]

Yeah ^ was the best way I could think of as well.

[dvv]

@v0idpwn interesting. But it forces to change the output format so that one needs to pipe through |> elem(0) everywhere.

[josevalim]

Let's continue the discussion here.

@v0idpwn interesting. But it forces to change the output format so that one needs to pipe through |> elem(0) everywhere.

You would only need to do a Enum.map/2 once right after you run the query. Since you have a helper function that builds a query, you could also have a function that runs the query and maps the results, this way you could strip the additional columns added by distinct.

However, that's still fetching unnecessary data from the database, so I would explore writing the query as a subquery or using another database feature, such as from p in subquery(the_actual_query_with_order_by), distinct: true.

[dvv]

It can not wrap output format in tuple because there's no custom place in %Ecto.Query{} to store the instruction to pipe through |> elem(0) once after the fetch.
Passing processing instructions out-of-band breaks composability.
Maintaining own copy of Ecto is deadend.

[Hyftar]

I encountered the bug as well. I ended up adding a virtual field to the struct to be able to use select_merge, which is not ideal either.

[greg-rychlewski]

@josevalim I took a look at this one again to see if I could think of anything and I think we should close it. There is a fundamental problem with the suggestion that I overlooked the first time around:

Repo.all(from foo in Foo, join: bar in assoc(foo, :bar), order_by: [asc: bar.name], distinct: true)
# SELECT DISTINCT f0.id, b1.name FROM foo AS f0 INNER JOIN bar AS b1 ON b1.foo_id = f0.id ORDER BY b1.name

The Ecto query is saying "give me all distinct records in Foo and choose the one with the lowest value of name in Bar. Now the proposed SQL will send back multiple records of Foo if they match multiple records in Bar:

SELECT DISTINCT f0.id, b1.name FROM foo AS f0 INNER JOIN bar AS b1 ON b1.foo_id = f0.id 

So basically you will get the wrong result set unless you add the de-dup logic to Ecto itself.

Postgres lets you order by columns that aren't select using distinct on which we already support. So that seems like the right thing to do here.