Add with_ties/3 to Ecto.Query

[greg-rychlewski]

Companion ecto_sql PR: elixir-ecto/ecto_sql#471. The integration tests fail here but pass there because I was hesitant to update the earthfile.

Background

The SQL standard version of the LIMIT command is FETCH. The commands are almost identical except FETCH comes with an option WITH TIES which says that the result should include any records that are tied with the last record. For example LIMIT 3 might return 1, 2, 3 while FETCH 3 WITH TIES would return 1, 2, 3, 3 because 2 records have the number 3.

Proposal

Add a :with_ties option to the current limit macro. There are a few reasons I'm proposing reusing the existing macro instead of creating a new fetch macro:

1. There is already normalization happening in the SQL Server adapter, which has FETCH and TOP but not LIMIT.
2. If there are 2 macros then we need to decide whether to normalize both to each other in situations where they are the same and an adapter supports one but not the other. Normalizing both ways increases the code complexity and number of decisions we have to make. And if we decide not to normalize then we need to justify why we're already doing it for SQL Server.

This proposal has a couple difficulties:

1. limit was using the generic %QueryExpr{} struct so doesn't have a place to store this new option. If we create a new struct for limits then any adapter pattern matching on %QueryExpr{} instead of %{} will have to be updated. All the built-in adapters had to be updated and I noticed SQLite will have to be as well.
2. Because the signature will be limit(query, bindings \\ [], expr, opts \\ []) it's a little awkward specifying the option outside of keyword queries if you don't care about bindings. But this would be the case for a new macro as well.

Reference

The postgres docs talk about it here: https://www.postgresql.org/docs/current/sql-select.html. It's the only built-in adapter that supports FETCH WITH TIES. SQL Server supports FETCH but not WITH TIES option. And MySQL supports neither.

[v0idpwn]

Not trying to impose, just asking: have you considered making a fetch macro instead of adding an option to limit?

[greg-rychlewski]

Not trying to impose, just asking: have you considered making a fetch macro instead of adding an option to limit?

@v0idpwn I did and put some discussion here:

• There is already normalization happening in the SQL Server adapter, which has FETCH and TOP but not LIMIT.

• If there are 2 macros then we need to decide whether to normalize both to each other in situations where they are the same and an adapter supports one but not the other. Normalizing both ways increases the code complexity and number of decisions we have to make. And if we decide not to normalize then we need to justify why we're already doing it for SQL Server.

But I'm completely open to doing a separate macro. Whatever the general consensus is for what's best. What do you and @josevalim think?

edit: Oh I just saw Jose suggest this

There is another approach here, which is to introduce a with_ties macro and it can only be called if a limit is set. It is similar to the recursive_ctes approach. :) I think it may be cleaner?

I'm open to this as well but didn't consider it before. I can't remember if that can be used in kw queries or not.

[josevalim]

Not trying to impose, just asking: have you considered making a fetch macro instead of adding an option to limit?

at the moment, fetch means limit, so I think a separate fetch macro is not necessary. We can reevaluate in the future if more features are added. :)

I'm open to this as well but didn't consider it before. I can't remember if that can be used in kw queries or not.

The KW queries can translate to anything we want. :D So I would keep basically the same code that you have in this PR, you would simply emit limit |> with_ties.

[josevalim]

Two minor nits and ship it!