Ecto: Support for `NULLS {FIRST|LAST}` in `order_by`

[hauleth]

This could help with writing some non-standard orderings.

However I have no idea how example syntax could look like. I thought about:

order_by(query, desc: {column, :nulls_last})

and

order_by(query, [{:desc, column, :nulls_last}])

And I like the first one more, however I do not know how about others.

[josevalim]

can't you use fragments?

[hauleth]

@josevalim I can. To be honest I am using them awfully lot (I am currently writing bunch of libraries to simplify using build in functionalities in SQL via fragments), however I think that such think should be allowed in Ecto natively.

Main point for that is about unifying syntax of my window/2 macro with native Ecto syntax. So if there would be will to add support for that in Ecto itself, then I would natively support it beforehand.

[josevalim]

@hauleth the tricky part here is the supporting it across databases.

PG and Oracle support NULLS FIRST AND NULLS LAST. ASC is the same as ASC NULLS LAST and DESC is the same as DESC NULLS FIRST.

MySQL does not support any of that and ASC is the equivalent to ASC NULLS FIRST. So we could introduce :asc_nulls_last and :asc_nulls_first and say that :asc defaults to the database ordering but I am not sure if that is the best API.

[hauleth]

@josevalim on MySQL you can mock that by using -column DESC however I do not know how this works with non numeric values.

[josevalim]

@hauleth this page is very useful! :+1: Given that:

The standard doesn't specify how NULLs should be ordered in comparison with non-NULL values, except that any two NULLs are to be considered equally ordered, and that NULLs should sort either above or below all non-NULL values.

I think we need to add all of asc_nulls_first, asc_nulls_last, desc_nulls_first and desc_nulls_last and say that without the qualifiers the order is database dependent (and MySQL can only implement two of those).

[schnittchen]

For the record and completeness,

order_by(query, [asc: fragment("? NULLS FIRST", column)]

seems to be the way this can be solved right now.

[josevalim]

Given that NULLS FIRST and NULLS LAST work with fragments, that's the approach I would recommend. Two simple macros can solve this problem and be nicely composable too:

defmacro nulls_first(column) do
  quote do: fragment("? NULLS FIRST", column)
end

defmacro nulls_last(column) do
  quote do: fragment("? NULLS LAST", column)
end

and then:

order_by(query, asc: nulls_first(column))

That's simpler then adding a bunch of new order_by types. Thanks @schnittchen and @hauleth!

[michalmuskala]

@josevalim have you checked it works? This will produce col NULLS FIRST ASC and the expected syntax is col ASC NULLS FIRST

[josevalim]

I went by @schnittchen's comment but I think you are right, we need to support it in Ecto.

[beerlington]

@josevalim can the new order_by directions (i.e. desc_nulls_last) be used with sorted distinct queries too?

I don't see anything in https://github.com/elixir-ecto/ecto/commit/0c216e00abbbe71dd9711df7b21b8ffe4ab1cc31 that explicitly addresses it, but I'm wondering about something like this for example:

q
|> distinct([g, ps], [
  desc_nulls_last: ps.sourcing_completed_at,
  desc: g.id,
])

I'm trying to do this now with fragments, but Ecto isn't removing the "NULLS LAST" part from the distinct clause and it's causing invalid queries. So something like this currently doesn't work:

q
|> distinct([g, ps],
  fragment("? DESC NULLS LAST, ?", ps.sourcing_completed_at, g.id)
)

[josevalim]

Definitely @beerlington! Could you please send a pull request? Please let me know if you have any questions!

[josevalim]

Actually, I did some tests and it seems this just works™! So I pushed two unit tests. We are good to go!

[beerlington]

@josevalim thank you for confirming! Is there any sort of workaround for the 2.2 branch or should I just fork my own branch and port over what I need from that commit?

[beerlington]

@josevalim that commit cherry picks almost perfectly into 2.2 so I'm just going to use a fork (assuming you don't want to back port that). Thanks!

[josevalim]

A branch is the way to go for now since we are not releasing new features in the v2 branch.