Enhance distinct error

[abdelhalim97]

Related issue

elixir-ecto/ecto#4671

Changes

Changing the error message when distinct has more than a field

[josevalim]

Thank you! But won't this exact case already be caught by the expression below?

[abdelhalim97]

The difference is that the query param has to have select *, this is being interpreted by the query match pattern %Ecto.Query{select: %SelectExpr{expr: {:&, [], [0]}}}

I think this AST {:&, [], [0]} means user is not trying to select specific fields
Thanks for your review

[josevalim]

Yes, but it will already raise with the error message below. Perhaps we should change the error below instead to say use distinct: true instead?

[greg-rychlewski]

Ecto actually selects the fields from the schema individually. So even if you don’t specify the fields yourself Ecto will. It won’t do select *.

[abdelhalim97]

Ah sorry i got it now you mean this case

defp distinct(%ByExpr{expr: exprs}, _sources, query) when is_list(exprs) do
      error!(query, "DISTINCT with multiple columns is not supported by MySQL")
end

Why i didnt think about it because in sql you cant do something like this anyway:
SELECT col1,col2 DISTINCT col1, col2
but you can do this
SELECT DISTINCT col1, col2

First case equivalent to distinct(%ByExpr{expr: exprs}, _sources, query) when is_list(exprs)
Second equivalent to the new one with the SelectExpr pattern match which should work in Mysql

---

What do you think? should i delete new pattern match and simply just change the below case msg error
or make ecto works with the new case with new pattern match like its working in Mysql?

[abdelhalim97]

@greg-rychlewski Thanks for the clarification

[josevalim]

Sorry, to clarify, I am saying that, instead of adding a whole new clause, you could update the existing one to add more information. There is no reason to have two very similar code paths. :)

[abdelhalim97]

Done 🫡