#78 order by of a has many through not respected

[peaceful-james]

I believe this fixes #78

[benwilson512]

Thanks for the PR! Please run mix format and then we should be good to go.

[benwilson512]

Interesting! So is the reason that this is required is because it can have a separate ordering from the overall query? I'm curious about the performance implications of this.

[peaceful-james]

To be completely honest, I don't really know.

Last night was my first time reading this code. I was encountering the bug in my side-project.
I just saw that this if expression was a critical branching point. My has_many...through...order_by expressions were being swallowed by repo.preload and lost somewhere in there. So I decided to try to make them got through the lateral_preload path. When it fixed my problem, and no other tests were failing, I opened the PR.

While I'm being completely honest, I should say that all the tests pass even if I write

raise "This path is not tested"

where the repo.preload call is (the negative case of the if expression).

So that flow is completely untested.

[peaceful-james]

Thanks for the PR! Please run mix format and then we should be good to go.

I ran mix format and nothing changed. I am using elixir 1.13, as per the .tool-versions file.
It seems the Github workflow is using an old Elixir versions (1.10).
I can update that as part of this PR?

[benwilson512]

Ah ok, I'll merge and then handle in master. Thanks!

[benwilson512]

Coming back to this way after the fact: One thing that is odd now is that if you specify order_by on anything it always uses the lateral option, which is pretty inefficient. It can also lead to invalid queries if the association being loaded also does joins. I'm gonna take another crack at this problem and see if I can refine down when path gets triggered.

[peaceful-james]

it always uses the lateral option

Yes I knew this would be the case. At the time I thought "better to be slow and reliable than fast and unreliable".

can also lead to invalid queries if the association being loaded also does joins

Can you write or at least envision a failing test to demonstrate this?

[tlvenn]

Updated our project to use the latest dataloader and now we have many preloading coming empty of things being dataloaded.

After some debugging, it appears that it's because of this change which forces the loads of many things through lateral option.

Trying to understand why the preloads are not working at all as soon as the query is ordered, it appears that the code that test for the existence of preloads in the query somehow does not work...

The query inspected:

#Ecto.Query<from a0 in Jumpn.Asset,
 left_join: c1 in assoc(a0, :current_version),
 left_join: p2 in assoc(a0, :pending_version),
 where: a0.purpose == ^:activity_gallery, order_by: [asc: a0.index],
 preload: [current_version: c1, pending_version: p2]>

The test on query.preloads actually returns [] even though the inspect above clearly shows the existence of preloads.

[benwilson512]

It is likely we're going to have to revert this commit for a few reasons.

1. As @tlvenn notes, it seems to introduce its own correctness issues
2. The performance penalties on ordered collections aren't practical for us (and probably others operating at a larger scale).

We have been using https://github.com/absinthe-graphql/dataloader/tree/remove-undesirable-lateral for a while now internally.

Notably, for through associations that are only through a single thing you can replace:

has_many(:liking_users, through: [:likes, :user])

with

many_to_many(:liking_users, User, join_through: Like)

And then when you specify an order on the User it is preserved. I plan to open an issue on Ecto to discuss some of this since honestly I think this is on Ecto.