Support subqueries in order_by, group_by, distinct, and windows

[zachdaniel]

So this seems to work without too much change required, but I do have one main misgiving: This requires a modification to Ecto.QueryExpr, which AFAIK is used in other places that won't be anticipate having subqueries in them.

I've got a test that shows this working as-is with ecto_sql: elixir-ecto/ecto_sql#607

[josevalim]

So this seems to work without too much change required, but I do have one main misgiving: This requires a modification to Ecto.QueryExpr, which AFAIK is used in other places that won't be anticipate having subqueries in them.

Let's create a new struct called ByExpr and use the opportunity to support subqueries in both group by and order by?

[zachdaniel]

Happy to do that, only concern with doing that will be anywhere that was explicitly matching on QueryExpr, but we can suss those out as necessary. Let me give it a try and see if anything breaks :)

[zachdaniel]

Alright, made some adjustments. There are three outstanding issues:

1. now this does require changes in ecto_sql. I'm working on those changes. They will have to be updated in tandem for this to work.
2. I might need some guidance on what to do with the subqueries/aliases present when building windows. I can't say for sure, but it seems like it might just need more updates in ecto_sql.
3. will need to add tests for subqueries in group_by and (when we figure it out) windows.

If we're good to move forward with this, then I will flesh out the tests both here and in ecto_sql.

[zachdaniel]

Working on wrapping this up today. I'm ending up doing windows support because windows uses the same code that group_by uses, so adding subquery support to that naturally flows on to adding it for windows (w/ some extra bits required to make that work).

The big thing I'm realizing based on your comment about testing non-dynamic cases is that I definitely haven't handled the compile time builder side of things. I'm going to try to do that as well, but I'm not sure if I'll be able to pull it off w/o some tips. I'll follow up and we can see how far I get 👍

[zachdaniel]

Still have some equivalent tests to write in ecto_sql. I'm curious if the non-dynamic/dynamic split is important to test in ecto_sql considering that it's tested here? Regardless, still need to test each query building scenario.

[zachdaniel]

Just commenting to point out that this should be ready to review. I can't see (for some reason) how or why the integration tests failed, but I'm assuming that it is because there are required changes in ecto_sql for this to work. Can we ship them simultaneously to address that? Do I need to find a way to make them compatible?

[greg-rychlewski]

@zachdaniel Normally we have the PR in ecto_sql point here, then if the integration tests pass there we know we are good and leave this one alone.

[zachdaniel]

@greg-rychlewski like I'd update the mix.exs to use this branch as a dependency?

[greg-rychlewski]

yeah exactly

[zachdaniel]

Done :) its building now.

[josevalim]

Looks great to me. Any other comments @greg-rychlewski? :)

[greg-rychlewski]

looks good to me :)

[josevalim]

💚 💙 💜 💛 ❤️