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

Handle joins with preloads #45

Closed
wants to merge 2 commits into from

Conversation

astjohn
Copy link

@astjohn astjohn commented Jan 8, 2018

Further to #44, here is a pull request that attempts to fix queries that have joins with preloads. Implementation was provided by @TisButMe a while ago in this PR but it seems like it never made it into recent versions.

The caveat is that it was necessary to avoid any queries with group_by.

Please let me know what you think.

@drewolson
Copy link
Owner

drewolson commented Jan 20, 2018

Apologies for the delay in review. I've been thinking a lot about this PR and I'm honestly unsure if we should merge it. I much prefer the current subquery strategy as it counts the exact query being run (without the uncountable clauses). I also feel like it is a bit of an anti-pattern to rely on include to perform a join and modify the count of the result set.

Thoughts?

@astjohn
Copy link
Author

astjohn commented Jan 22, 2018

No problem at all. I'm also swamped so apologies not necessary! Maybe it's just my naivety then and a matter of documentation.

How would we make the test pass that I added on line 355 of query_test.exs using a subquery? At the moment, it will fail without these changes.

@drewolson drewolson closed this Nov 8, 2018
@astjohn
Copy link
Author

astjohn commented Feb 4, 2019

Hey @drewolson . Coming back to this... I just realized you closed it, but did you get the test I added to pass using a subquery. Would love to know how that works. Thanks.

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.

None yet

2 participants