Support preloading associations in embedded schemas from the parent schema

[Sleepful]

Implements enhancement #3890

Re-submission of #3965

Might add more tests later

[Sleepful]

Added all requested changes.
The test function is getting a little long but it is all closely related so I'm leaving it tightly coupled.

Offtopic: I am listening to the Thinking Elixir Podcast and learning some interesting tidbits from the Elixir Review conversations @josevalim 😁

[greg-rychlewski]

I have a suspicion the parallel preloads aren't working as expected, but need more time to look at the code to be sure.

It looks to me like anytime there is an embed it will have its assocs preloaded in isolation from all the other embeds and assocs. And if there are nested embeds then only a single "level" of assocs will be preloaded at once.

I could be wrong though. But that's what it looks like at a glance.

edit: nm I was being a newb. this is how it works for regular assocs too

[josevalim]

@greg-rychlewski is this good to you? :)

[greg-rychlewski]

oh not yet sorry. they are still working on batching the preload_embed function: #3967 (comment)

[Sleepful]

had to leave this on hold for a while, taking more stabs at it every now and then though

[Sleepful]

yep this is proving difficult with the given requirements so it will take awhile longer but I think I'm on the right path, the thing is I need to wire the embeds into the prepare_queries function and it's not as simple as bundling the embeds with the other assocs because the embeds require different arguments structs, module when calling prepare_queries(structs, module, .....

Sorry if that doesn't make sense, it is WIP

[greg-rychlewski]

How come prepare_queries needs to be changed? I think it should be working already because preloading embeds at the root works.

From what I can tell the only missing part is mimicking this part of preload_assocs:

all = preload_each(Enum.reverse(loaded_structs, fetch_structs), repo_name, preloads, tuplet)
entry = {:assoc, assoc, assoc_map(assoc.cardinality, Enum.reverse(loaded_ids, fetch_ids), all)}
[entry | preload_assocs(assocs, queries, repo_name, tuplet)]

• The first line you will pass the embed structs and it will preload all their assocs.
• The second line you will reattach your embed with the preloads onto the original structs.
• The third line will recursively preload the next embed.

[greg-rychlewski]

Something like this:

defp preload_embeds(structs, [], _repo_name, _tuplet), do: structs

defp preload_embeds(structs, [embed | embeds], repo_name, tuplet) do
  {%{field: field}, sub_preloads} = embed
  embeds = Enum.map(structs, &Map.get(&1, field)) |> preload_each(repo_name, sub_preloads, tuplet)
  structs = Enum.zip(structs, embeds) |> Enum.map(fn {struct, embed} -> Map.put(struct, field, embed) end)
  preload_embeds(structs, embeds, repo_name, tuplet)
end

Sorry not optimized or tested but this is the gist of what I was thinking. You will also have to keep track of how many embedded structs there are per parent struct in case of embeds_many and also have to deal with nil.

edit:

Or maybe even nicer, to be the same as assocs, preload_embeds can return a list of {:embed, embed_reflection, something_to_assign_parent_to_preloaded_embeds} and then at the end of preload_each load the embeds the same as the other assocs:

for struct <- structs do
        struct = Enum.reduce assocs, struct, &load_assoc/2
        struct = Enum.reduce throughs, struct, &load_through/2
        struct = Enum.reduce embeds, struct, &load_embed/2
        struct
      end

I don't think parent has to have a primary key in case it only has embeds and no direct assocs. So something_to_assign_parent_to_preloaded_embeds might have to be a list of structs (embeds_one)/list of lists of structs (embeds_many) in the same order as the parents or something.

[Sleepful]

this line in fetch_ids (inside prepare_queries) is the one that will not accept an assoc which comes from an embed with the parent (meaning, the struct that contains the embed) struct as structs (the ones you called relevant structs):

        %{^owner_key => id, ^field => value} = struct

Right? Because you were telling me how preload_each should not be called for each embed'ed struct, because that would mean that preload_each gets called once per relevant struct

[greg-rychlewski]

I might not have explained myself properly. What I meant was that you extract the embeds from the parent structs and then pass just the list of embeds into preload_each. Wouldn't that be the same as passing the embeds themselves into preload_each even if there was no parent?

[Sleepful]

I might not have explained myself properly.

Don't worry, it's probably on me.

What I meant was that you extract the embeds from the parent structs and then pass just the list of embeds into preload_each.

Sort of, current implementation is more similar to this:

For each relevant struct: Extract embed, call `preload_each` for this embed

Wouldn't that be the same as passing the embeds themselves into preload_each even if there was no parent?

It is exactly that, passing the embeds themselves into preload_each, this preload_each has no information about the arguments being "embeds".

So, if you don't mind, can we come back to the previous comment again?

You mentioned that current implementation ties up more DB connections than needed:

Ecto advertises that it's one extra connection per assoc but now you have multiple connections for a single assoc

How does this happen?

Lets say we have [RelevantStructs] which embed EmbeddedStruct and these embeds contain EmbeddedAssoc. Current implementation will call preload_each once for each EmbeddedStruct, which may be multiple calls because that would be once for each RelevantStruct.

Should it instead call preload_each once with all of the [EmbeddedStructs]?

[greg-rychlewski]

Should it instead call preload_each once with all of the [EmbeddedStructs]?

Yeah this is exactly what I was trying to propose here: #3967 (comment).

So you call preload_each once with all the embeds and then reattach them to the parents with all the assocs preloaded.

[Sleepful]

Ah, perfect! Thanks 😁

I misunderstood at first and thought all embeds had to be loaded on the same connection as the other assocs. Oops!

[Sleepful]

Hi! I think I got it (crossing fingers), one thing I did not have to do is deal with any nil values which you mentioned here #3967 (comment), so please let me know if I am missing a nil edge case.

Other than that I believe this is now calling prealod_each once per unique Ecto.Embedded in structs.

[Sleepful]

Do you have any tips for debugging elixir? I have been print-debugging every time I run the integration tests but it feels so clunky, I know iex has a lot of facilities (mostly unknown to me) but using iex seems so difficult, mostly because typing in the terminal is really tough compared to typing in the editor; iex even takes away some of my terminal's typing facilities, rlwrap didn't even help me enable arrow keys in iex to navigate my input . I have enjoyed using nREPLs in other environments like clojure, not sure if elixir can do anything similar.

[greg-rychlewski]

Thanks @Sleepful this is looking great. I just had 2 smalls suggestions for the unit test and one suggestion to refactor preload_embeds. But these should be quick to implement then we can merge :).

one thing I did not have to do is deal with any nil values which you mentioned here #3967 (comment), so please let me know if I am missing a nil edge case.

It looks like this was my confusion. I assumed preload_each wouldn't allow a nil value for a struct but it does. I updated my refactoring suggestion to account for this.

Do you have any tips for debugging elixir? I have been print-debugging every time I run the integration tests but it feels so clunky, I know iex has a lot of facilities (mostly unknown to me) but using iex seems so difficult, mostly because typing in the terminal is really tough compared to typing in the editor; iex even takes away some of my terminal's typing facilities, rlwrap didn't even help me enable arrow keys in iex to navigate my input . I have enjoyed using nREPLs in other environments like clojure, not sure if elixir can do anything similar.

Sorry I'm pretty newb/lazy about tooling and don't really do anything special. But @josevalim might have some more information about this.

[greg-rychlewski]

Thanks @Sleepful, great work!.

[josevalim]

Awesome job @Sleepful with the code and @greg-rychlewski with the reviews! ❤️