order_by with distinct generates wrong query

[achempion]

Environment

• Elixir version (elixir -v):
  elixir:1.11.2-alpine
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.):
  postgres:13.1-alpine
• Ecto version (mix deps):
  3.5.5
• Database adapter and version (mix deps):
  postgres 0.15.8
• Operating system:
  linux alpine

Sample 1

Current behavior

from(i in Import, distinct: i.id, order_by: [desc: i.id]) |> Repo.all

generates

SELECT DISTINCT ON (i0."id") i0."id",  i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id", i0."id" DESC

Expected behavior

would generate

SELECT DISTINCT ON (i0."id") i0."id",  i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id" DESC

Sample 2

Current behavior

from(i in Import, distinct: [desc: i.id]) |> Repo.all

generates

SELECT DISTINCT ON (i0."id") i0."id", i0."inserted_at", i0."updated_at" FROM "imports" AS i0

Expected behavior

would generate

SELECT DISTINCT ON (i0."id") i0."id", i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id" DESC

Sample 3

Current behavior

from(i in Import, distinct: [desc: i.id], order_by: [desc: i.id]) |> Repo.all

generates

SELECT DISTINCT ON (i0."id") i0."id", i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id" DESC, i0."id" DESC

Expected behavior

would generate

SELECT DISTINCT ON (i0."id") i0."id", i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id" DESC

would generate section contains the same query in every example

[josevalim]

Sample 1 and Sample 2 behaviours are documented - and therefore I would not change it.

Sample 3 is the correct way to go about things, if you want to enforce both order by and distinct - but we need to remove the duplicate.

[achempion]

It seems non obvious to me that on Sample 2 we ignore distinct sorting option if order_by option is empty.

[dorian-marchal]

The distinct version of the sort order seems to have been added to avoid repeating columns in both distinct and order_by (cf. #35 (comment)).

But we now have to repeat both the columns and the sort orders, which is unexpected.
What do you think?

Maybe this should be allowed?

Current behavior

from(i in Import, distinct: [desc: i.id], order_by: []) |> Repo.all

generates

SELECT DISTINCT ON (i0."id") i0."id",  i0."inserted_at", i0."updated_at" FROM "imports" AS i0

Expected behavior

would generate

SELECT DISTINCT ON (i0."id") i0."id",  i0."inserted_at", i0."updated_at" FROM "imports" AS i0 ORDER BY i0."id" DESC