neo4jrb / activegraph

An active model wrapper for the Neo4j Graph Database for Ruby.
http://neo4jrb.io
MIT License
1.4k stars 276 forks source link

order with limit and with_association bug #1509

Closed klobuczek closed 5 years ago

klobuczek commented 6 years ago

Fixes # There is a bug when all 3 order, limit and with_associations is used. The limit is applied before order by. The main problem around order and with_associations is that the collect used in with_associations reshuffles already sorted set. So the order has to come at the end. Since however, the with_associations is very expensive since it potentially touches and returns a large amount of data it would be nice that the result is already limited before it is processed by with_associations, which requires order and limit happening before. The approach I have taken here is to repeat the order at the end of the query. Another approach would be to move the limit to the very end. Both approaches are functionally the same. But the former is more performant in general case. I am open for suggestions either way. But one thing is clear that it cannot stay the way it is as pagination would return totally wrong results.

This pull introduces/changes:

codecov-io commented 6 years ago

Codecov Report

Merging #1509 into master will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1509      +/-   ##
==========================================
+ Coverage   96.82%   96.82%   +<.01%     
==========================================
  Files         205      205              
  Lines       12688    12700      +12     
==========================================
+ Hits        12285    12297      +12     
  Misses        403      403
Impacted Files Coverage Δ
...o4j/active_node/query/query_proxy_eager_loading.rb 100% <100%> (ø) :arrow_up:
spec/e2e/association_proxy_spec.rb 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c579637...c02c415. Read the comment docs.

klobuczek commented 6 years ago

@cheerfulstoic could we move ahead on that? It is not ideal to point our production to forks and keep synching the fork with upstream.

cheerfulstoic commented 6 years ago

Hey @klobuczek

Unfortunately I haven't been able to continue maintaining the Neo4j.rb as I have other projects (see this issue.

I'll be happy to provide advice on whoever would take over. I have one person who's potentially interested.