[WIP] Add support for named joins #2411

zoldar · 2018-02-09T13:49:38Z

I took a stab at implementing support for named joins as discussed in #2389, using the ideas from initial post and #2389 (comment).

Example usage:

query =
  Post
  |> join(:inner, [p], {:comment, c} in assoc(p, :comments))
  |> order_by([comment: c], c.id)
  |> select([p, comment: c], {p, c})

Marking it as work in progress because I'm definitely not sure whether

the API is acceptable
it's implemented optimally.

Other than that, I'll want to refactor Query.Builder.escape_binding for sure, because there's a lot of redundancy and excessive nesting at the moment. Also, docs will need update eventually as well when/if the proposed change gets into acceptable shape.

Feedback appreciated.

sourcelevel-bot · 2018-02-09T13:52:31Z

lib/ecto/query/builder.ex

+            {query, vars ++ tail_fn.(tail)}
+          {tail, named} ->
+            mapped_binds =
+              Enum.map(named, fn {k, name} -> {k, quote(do: bind_mappings[unquote(name)])} end)


Function body is nested too deep (max depth is 2, was 3).

sourcelevel-bot · 2018-02-09T13:52:32Z

Ebert has finished reviewing this Pull Request and has found:

1 possible new issue (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2411.

josevalim · 2018-02-10T10:50:17Z

Thanks for the PR @zoldar! ❤️ 💚 💙 💛 💜

This is great I have a couple suggestions though.

I still prefer join c in "comments", as: :comments because it allows us to also use the :as option when generating SQL queries. It is more verbose but it is also clearer. For example, imagine reading join: {:comments, c} in {"comments", Comment} by using tuples it is really hard to figure out what is what.
The way you compute the names right now is very expensive because it is linear based on the number of joins. My suggestion is to add a new field to Ecto.Query called aliases and store it there. Seeing if a field exists or not should be a simple lookup.

Because this PR is potentially quite long, I would recommend breaking this into smaller tasks and we work step by step towards the end result:

The first step is to make Ecto.Query.join/5 support keyword list as last argument. Today join/5 expects the last argument to a list of expressions:

join(Post, :left, [p], c in assoc(p, :comments), c.public)

We need to change it to support a keyword list as last argument and emit a warning if a keyword list is not given. So we want this:

join(Post, :left, [p], c in assoc(p, :comments), on: c.public)

This will also be necessary for us to support index hinting (#2392) in the future. But for now we will support only :on anything else will raise.

Now that we made joins extensible, the second step is to add support to :as in both join and from. The aliases should be stored in the query, so lookups are fast. Trying to register an already given name should be an error. Trying to use a name that does not exist should be an error too.

Finally, as third step, we can include the :as option in the generated SQL statements too.

Thoughts?

josevalim · 2018-02-10T10:54:06Z

test/ecto/query_test.exs

@@ -268,6 +268,42 @@ defmodule Ecto.QueryTest do
    end
  end

+  describe "named joins" do
+    test "assigns a name to a join" do


Those tests are very fragile because they are asserting on the AST. The reason why we test them above is because we want to check if the query is optimizable but we are not worried about optimization here. My recommendation is to build actual queries and then traverse query.joins (or query.aliases if we follow my proposal) checking for the generated aliases, like you did in the dynamic tests.

It is also very important to test what happens when we give an alias that already exists. We also need to check what happens when you try to use an alias that does not exist.

josevalim · 2018-02-10T10:54:59Z

test/ecto/query/builder_test.exs

@@ -124,6 +124,13 @@ defmodule Ecto.Query.BuilderTest do
           escape(quote(do: ^([] ++ [])), [], __ENV__)
  end

+  test "escape_binding with named binding in the input" do


This is a fragile test too, we don't need to check the generated AST on expansion.

josevalim · 2018-02-10T10:55:49Z

test/ecto/query/builder/dynamic_test.exs

@@ -66,5 +72,29 @@ defmodule Ecto.Query.Builder.DynamicTest do
             "&0.foo() == ^0 and &1.bar() == ^1 and &0.baz() == ^2"
      assert params == [{"foo", {0, :foo}}, {"bar", {1, :bar}}, {"baz", {0, :baz}}]
    end
+
+    test "with ... binding in the middle" do


Those tests are beautiful! I would recommend moving them to query_test.exs as there is nothing specific to fragments about them. Have them work directly on the query and then check the result of query.joins (or query.aliases according to my proposal).

zoldar · 2018-02-10T14:23:08Z

@josevalim Thank you for the feedback. All the points you made are valid to me and make sense. If nobody else has any objections, I'd start with stage one, as described by you, some time next week.

zoldar · 2018-02-11T16:16:41Z

I need a word of advice. When I've started working on introducing keyword opts in join/5, I've hit an issue with the fact that join actually allows to accept keyword list for on expression here:

https://github.com/zoldar/ecto/blob/cea93504d0dd609ad92733692e5bdd6830b685a9/test/ecto/query/builder/join_test.exs#L23

In order for this to work, I guess it would be necessary to tell apart the case where it's meant to be treated as on expression from the one where it's to be treated as a keyword list of options. I'm not sure whether there's a sensible solution for that. The on expression could very well to refer to fields such as on or as. Any ideas?

josevalim · 2018-02-11T16:49:19Z

@zoldar yeah, you will have to check that it is either an empty list or it has as or on in it. There is a very small chance of a conflict but this is a major release so we can afford it.

Theoretically we could just go and break everything but that would be annoying for users. :)

zoldar · 2018-02-16T14:33:03Z

Closing in favor of plan laid out in #2411 (comment) and #2428. Leaving for reference though.

zoldar added 11 commits February 9, 2018 12:00

Allow to bind join to a name

6235c8c

Add support for named bindings

56f305a

Remove debug statements

0ea5ce9

Process named binds only when necessary

4a22ea8

Add tests, including dynamic queries

f7161e2

Go back to generating bind mappings in one place

043d9eb

Handle different join shape for dynamic query when escaping binding

8c4618d

Test Ecto.Query.Builder.Join.escape/3 more thoroughly

306d42d

Improve named join dynamic query tests

38508eb

Add named binding test case for Ecto.Query.Builder.escape_binding

0e42ea3

Add integration test

cea9350

sourcelevel-bot bot reviewed Feb 9, 2018

View reviewed changes

josevalim reviewed Feb 10, 2018

View reviewed changes

zoldar mentioned this pull request Feb 16, 2018

Support passing on to join/5 as option and soft-deprecate raw argument #2428

Merged

zoldar closed this Feb 16, 2018

zoldar mentioned this pull request Feb 19, 2018

[WIP] Add support for named joins #2431

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add support for named joins #2411

[WIP] Add support for named joins #2411

zoldar commented Feb 9, 2018

sourcelevel-bot bot Feb 9, 2018

sourcelevel-bot bot commented Feb 9, 2018

josevalim commented Feb 10, 2018 •

edited

Loading

josevalim Feb 10, 2018 •

edited

Loading

josevalim Feb 10, 2018

josevalim Feb 10, 2018

josevalim Feb 10, 2018

zoldar commented Feb 10, 2018

zoldar commented Feb 11, 2018

josevalim commented Feb 11, 2018

zoldar commented Feb 16, 2018

[WIP] Add support for named joins #2411

[WIP] Add support for named joins #2411

Conversation

zoldar commented Feb 9, 2018

sourcelevel-bot bot Feb 9, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot commented Feb 9, 2018

josevalim commented Feb 10, 2018 • edited Loading

josevalim Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

josevalim Feb 10, 2018

Choose a reason for hiding this comment

josevalim Feb 10, 2018

Choose a reason for hiding this comment

josevalim Feb 10, 2018

Choose a reason for hiding this comment

zoldar commented Feb 10, 2018

zoldar commented Feb 11, 2018

josevalim commented Feb 11, 2018

zoldar commented Feb 16, 2018

josevalim commented Feb 10, 2018 •

edited

Loading

josevalim Feb 10, 2018 •

edited

Loading