Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow map/2 and struct/2 to take subsets of embed fields #4558

Closed

Conversation

greg-rychlewski
Copy link
Member

@greg-rychlewski greg-rychlewski commented Dec 22, 2024

Closes #4536

The general idea is to select the fields individually and then combine them in Elixir after they are retrieved from the database. There are more corner cases than I was anticipating. Here are some points for consideration:

  1. I am using json_extract_path to select the individual fields from the embed assuming this abstraction is meant to work for most databases. If it's not mean to be, then is it better to create a general expression for embed fields and have the adapter convert it?
  2. Right now this only works for embeds_one and not embeds_many because of the way extractions are being done. There is a similar limitation on json_extract_path. There is a chance it can be extended to embeds_many but I am not sure there is a simple operator to extract the same key from every object in a json array. So if we want to keep this we might have to be ok with it only working for embeds_one
  3. Right now taking associations inside of embeds is not supported. I think this can be done but if it's not a big deal would prefer to do it in a follow-up PR. I think there are enough complications with the current PR to consider.
  4. There is a current inconsistency, maybe intentional, between taking associations and taking embeds that is carrying over to this PR. If doing map(p, [assoc_field: [:assoc_sub_field]) the :assoc_field will also be a map. But if it's an embed field it will be a struct. This happens with or without the current PR. I kept the current behaviour but wanted to highlight it in case we want to make these consistent in a different PR. It will probably be a larger change to see why it behaves this way today.

@josevalim
Copy link
Member

Thank you @greg-rychlewski, this looks neatly done but, after looking at the implementation, I wonder if it is worth it?

I am also worried about having partial representations of the data, because we only load part of the map or embedded schema. In a way this is expected, but this will become increasingly problematic with the type system, as we need to effectively assume all fields to be nil.

In this case, it is probably best to write:

select: {map(o, [:metadata]), o.item.price.primary_color.name}

@greg-rychlewski
Copy link
Member Author

after looking at the implementation, I wonder if it is worth it?

Yeah I agree. I had a hard time letting it go but I was also thinking the same thing after finishing. Thanks for reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Selecting parts of embeds with Ecto.Query.API.struct/2
2 participants