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

Fix select_merge on map with outer join #3219

Merged
merged 2 commits into from
Feb 3, 2020
Merged

Fix select_merge on map with outer join #3219

merged 2 commits into from
Feb 3, 2020

Conversation

halostatue
Copy link
Contributor

Resolves #3218

Three cases have been added to merge processing:

  • If the merge argument (right) is nil, return left unmodified. This can
    happen with a :left_join or :full_join. This is fully tested.

  • If the merge source (left) is nil, return right unmodified. This can
    happen with a :right_join or :full_join. This is not tested.

  • If both the source and argument are nil, return nil. This probably
    shouldn’t happen, but is currently included for completeness. This is not
    tested.

Resolves #3218

Three cases have been added to merge processing:

- If the merge argument (`right`) is `nil`, return `left` unmodified. This can
  happen with a `:left_join` or `:full_join`. This is fully tested.

- If the merge source (`left`) is `nil`, return `right` unmodified. This can
  happen with a `:right_join` or `:full_join`. This is not tested.

- If both the source and argument are `nil`, return `nil`. This _probably_
  shouldn’t happen, but is currently included for completeness. This is not
  tested.
@josevalim
Copy link
Member

josevalim commented Feb 2, 2020

Thanks @halostatue, the PR looks great. Just one comment:

We generally like to avoid adding new fixtures/schemas/tables to integration tests, otherwise they would grow very unwieldy with time. Perhaps a simpler to add a title field to Permalink? This way Post in your tests become Permalink and Post.Translation becomes Post. You can use any of the posts fields to make the join return a result or not. Could you please amend it accordingly?

@halostatue
Copy link
Contributor Author

Let me see what I can do. It will be much later today before I can get to it.

@halostatue
Copy link
Contributor Author

Done. I did add one field to permalink so that the distinction between overwriting an existing field and a virtual field are considered in the tests here.

@josevalim josevalim merged commit 327a094 into elixir-ecto:master Feb 3, 2020
@josevalim
Copy link
Member

Thank you, fantastic work! 💚 💙 💜 💛 ❤️

josevalim pushed a commit to elixir-ecto/ecto_sql that referenced this pull request Feb 3, 2020
@halostatue halostatue deleted the handle-merge-map-outer-join branch February 3, 2020 19:21
zachdaniel pushed a commit to zachdaniel/ecto_sql that referenced this pull request Sep 19, 2020
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.

select_merge + map + join(:left) fails by returning nil instead of map
2 participants