order_by + distinct builds wrong query

[fenollp]

Environment

• Elixir version (elixir -v):

Erlang/OTP 21 [erts-10.1.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe] [dtrace]

Elixir 1.7.4 (compiled with Erlang/OTP 21)

• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): PG_VERSION=9.6.6
• Ecto version (mix deps):

* ecto 2.2.11 (Hex package) (mix)
  locked at 2.2.11 (ecto) 4bb8f117

• Database adapter and version (mix deps):

* postgrex 0.13.5 (Hex package) (mix)
  locked at 0.13.5 (postgrex) 3d931aba

• Operating system: OSX 10.14.1

Current behavior

This code:

queryable
|> order_by([desc: :begin_at, desc: :id])
|> distinct([:begin_at, :id])

generates the following request:

SELECT DISTINCT ON (m0."begin_at",
                    m0."id") m0."id",
                   ...)
...
GROUP BY m0."id"
ORDER BY m0."begin_at",
         m0."id",
         m0."begin_at" DESC,
         m0."id" DESC

Expected behavior

SELECT DISTINCT ON (m0."begin_at",
                    m0."id") m0."id",
                   ...)
...
GROUP BY m0."id"
ORDER BY m0."begin_at" DESC,
         m0."id" DESC

It appears there is a difference between these two way of writing the order_by()s.

See elixir-ecto/ecto#504 for initial discussion on distinct_on+order_by.
See elixir-ecto/ecto#1669 for what seems to be a very similar bug report.

Note that I explicitly select distinct order_by fields to avoid

SELECT DISTINCT ON expressions must match initial ORDER BY expressions

Current workaround:

1. take out order_bys
2. convert to_sql
3. generate correct text from order_bys

EDIT: I haven't tried ecto_sql v3 but code in connection.sql looks similiar enough.

[fenollp]

Actually I have a working fix at: elixir-ecto/ecto#2810
Running lots of tests right now.

[michalmuskala]

Since 2.2 ecto supports adding asc and desc directions to the distinct column. I believe this would be a proper way to handle those in here.

For example:

queryable
|> distinct([desc: :begin_at, desc: :id])

[fenollp]

Amended my PR to prefer ordering from distinct as suggested above.

[josevalim]

@fenollp what @michalmuskala meant is that this is working as expected. The solution is not to duplicate the order by and the distinct, is to use |> distinct([desc: :begin_at, desc: :id]) in the first place. I don't think we should remove the duplicates, especially ones added by the user.

Does it make sense?

[fenollp]

Ah I get it now. Indeed this works fine! Thanks