Add map/1 to Ecto.Query.API

[greg-rychlewski]

Motivation

It seems it helps with LiveBook: https://groups.google.com/g/elixir-ecto/c/8E8MGOma-XM.

Implementation

There were 2 options I thought were possible:

1. Change the select expr to signify it is a map.
2. Change the take value to signify we want all the fields

This PR was done with option 2. I thought the blast radius would be a lot larger for option 1.

Basically I am setting take to be nil if map/1 is used. I believe this is safe because otherwise it's guaranteed to be a list. Also nil is safer over other atoms because sometimes Access.fetch is called on take to find association fields.

[josevalim]

I wonder if implementation wise we could get all fields from the schema and make it equivalent to map(p, all_fields_from_p)?

[greg-rychlewski]

I can take a look. I think when you are building the query it's not that simple to get the schema without some decent sized changes. But I will try more earnestly to make that work.

[greg-rychlewski]

The main deterrent I see is that we need to access query.from.source which might not be expanded while building the query.

So I believe the only option is to build map/1 in some intermediate step and then do a pass during runtime to change it to map(p, all_fields_from_p). Then I think it also means you can't keep the query compile time while using map/1 otherwise you are passing around an intermediate expression you want to change.

Hopefully I'm not talking complete nonsense 😅 . If all of the above is true then I'm not sure it's better than what is in this PR. But open to feedback.

[josevalim]

I was thinking we would expand like that in the planner. This way we don’t need to add the new nil cases. :)

[greg-rychlewski]

Sorry you said planner. Deleted my other comment. I get it now.

[greg-rychlewski]

I think I got it working without too much damage. The biggest change is that we need to accumulate the take when prewalking through the query in the planner. Because map/2 is essentially expanded to {:&, _, [ix]} plus adding the fields to the take.

[josevalim]

Instead of expand it here, I think you could say in classify_merge that it has type {:map, meta, :runtime}.

[greg-rychlewski]

Oh yeah I tried this but then it makes it less powerful than map/2. For instance you can do this with map/2:

from c in Comment, select: map(c, [:id]), select_merge: [:dislikes]

But if I don't expand map/1 here it will complain that a source can't be merged into a map when doing this

from c in Comment, select: map(c), select_merge: [:dislikes]

[greg-rychlewski]

I could make it something similar {:map, meta, something_about_map/1} to catch this case?

[josevalim]

I think for subqueries it doesn't matter. This can be treated exactly the same as {:&, _, [ix]} because we never return a struct or a map. Can you try something like:

  defp subquery_select({:map, _, [{:&, _, [_]} = entry]}, take, query) do
    subquery_select(entry, take, query)
  end

?

[greg-rychlewski]

I remember why I did this now. If you try this integration test:

@tag :map_1
  test "map/1" do
    p = TestRepo.insert!(%Post{})
    q = from p in Post, select: map(p)
    IO.inspect TestRepo.all(subquery(q))
  end

It will return a struct because the take is not tagging the fields as :map.

[josevalim]

Why did we need those changes to prewalker? In any case, the acc is opaque, so if you need a counter, you should include the counter in the accumulator. :)

[josevalim]

I think I understand. counter is the previous acc but now you need to return the expression with an updated take. Let me think about it.

[josevalim]

Here is any idea: what if instead of changing prewalk to expand map/1, you change collect_fields to consider map/1? You can test it out this way:

1. Remove the {:map, _, [_]} clause from prewalk
2. Add the clause to collect_fields
3. If it works, you can revert all changes to prewalk (probably easier to revert the file :D)

[greg-rychlewski]

oh actually I tried the collect_fields route first and then I noticed something broke here

ecto/lib/ecto/query/planner.ex

Line 1387 in 8c232cc

{tag, from_take} = Map.get(take, 0, {:any, []})

particularly with preloads. i'll reproduce to get your opinion on it

[josevalim]

Hrm, it seems I should be the one giving it a try then. :)

[josevalim]

@greg-rychlewski I pushed two small fixes and some code comments to collect fields. I made merge itself recursive and removed an outdated :never check. This means that supporting map/2 and struct/2 on collect fields is easier, as we only need to add two clauses (one for map and another for struct) instead of several closes that consider how merge and other factors interplay.

[greg-rychlewski]

beautiful changes! I'l give the refactoring a try and push them here

[josevalim]

Let’s do the refactor in a separate PR and then add this functionality later so we don’t mix them. :)

[greg-rychlewski]

Btw sorry I forgot to ask yesterday. Do you mean completely remove the take system or just for map/structs? For instance for this:

from p in Post, select: [:title, :id]

Would we want to represent it as something like this {:source, _, [{:&, _, [ix]}, [:title, :id]]}? I like the idea of completely getting rid of take but not sure if you saw a reason to keep this one.

[josevalim]

We will keep the system but instead of storing it as a separate field we keep all info in the AST. We can rewrite the case you posted as map_or_struct(&ix, […]) (if we have :all we can wrap all sources in that but maybe once we do it we will see the benefits of keeping it separate).