elixir-lang/ecto — pull request #151 by Enders (2014-01-01 11:04:01+00:00)
Title: [WIP] Distinct: add basic distinct to the query DSL (+ add to PG adapter)

This is a WIP.

- [x] (1) functionality as described in issue #93's (DISTINCT ON)
- [ ] (2) ORDER BY issue when using SELECT DISTINCT ON in PG
- [ ] (3) factoring the similar code between GROUP BY and DISTINCT (builder, some utility functions)
- [ ] (4) documentation (marked as #TODO in query.ex)

OK, so this is a WIP but as it is a 'complex' feature (for me!) I would love to have someone take a first look at what's going on here!

I have put a 'x' in the first task because the generated SQL is what was expected in issue #93 discussion, basic tests have been added and a check on the list of fields in the distinct has been added in case the select expression specify a different set of fields (see validate_distincts/2, it took me a while to get it right and I am worried it went horribly wrong).

BUT! I have a problem with the validator being at the DSL level and not at the adapter level, which is task 2 above. In a few words: SELECT DISTINCT ON doesn't play nice with ORDER BY in PG. For instance, the following query would fail:

postgres=# SELECT DISTINCT ON(city) city, temp_lo FROM weather ORDER BY temp_lo;
ERROR: SELECT DISTINCT ON expressions must match initial ORDER BY expressions

Without the ORDER BY, it obviously works. But when there is an ORDER BY, it must contain the fields from the DISTINCT ON and they must be on the left most part of the ORDER BY expression. That's a complex validation... but more importantly, it's PG specific. Other adapters may not have DISTINCT ON and so maybe we will just use SELECT DISTINCT, which brings other kind of limitations... but not this one.

So there is an apparent conflict between the validation being adapter independent and the fact that adapters will add random constraints on some of the key words from the generic DSL. This probably came up in earlier discussions, I'd love to get up to speed on this!

I see 4 ways to deal with this....

- Implement both a distinct and distinct_on in the DSL but the obvious issue is that it'll be hard (but probably not impossible) to implement distinct_on in every adapter
- Implement distinct with SELECT DISTINCT ... rather SELECT DISTINCT ON(...) but then we cannot do the distinct on a subset of field (which is the reason for this whole mess to begin with) and there are other constraints with SELECT DISTINCT anyway, so this is just pushing the problem
- Delegate more validation to the adapter ... sounds like a big task given the current structure of Ecto, unless I missed a step somewhere?
- Keep things as is, and let PG fail when someone uses distinct\2 and an incorrect order_by\2 because that's all they deserve.

I am voting for the last one but I am hoping that someone will have an idea to do something along the lines of the third one. Any other idea?

As for task 3 in the list above, well, it's fairly obvious from the code, some bits have been shamelessly copied from the group by logic. It could be refactored... but I am not sure that's a good idea -- distinct and group_by act similarly in some way, but are still very different and the smarter ecto will be, the more small differences will creep-in... so I'd rather leave things as separated as possible. What's your opinion?