-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Automatically set first order_by when distinct set #504
Conversation
@@ -213,12 +213,16 @@ if Code.ensure_loaded?(Postgrex.Connection) do | |||
end | |||
end | |||
|
|||
defp order_by(order_bys, sources) do | |||
defp order_by([], _distincts, _sources), do: nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If order_by is empty, we should still generate the order by for distincts.
It would be more efficient to build the SQL for distinct once and reuse that in order_by. Would that be possible? |
@ericmj yeah. |
25f0282
to
68e8ef6
Compare
@josevalim @ericmj are the latest changes okay? |
"" -> nil | ||
_ -> "ORDER BY " <> exprs | ||
case {distinct_exprs, exprs, distinct} do | ||
{_, "", _} -> nil |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericmj said no. :)
Automatically set first order_by when distinct set
❤️ 💚 💙 💛 💜 |
Thanks, guys! 👍 |
Addresses this issue: #495
My initial implementation for this enhancement but will need to add docs and test it some more.