Better detection of standard preload #160
Conversation
kbrock
force-pushed
the
preload_array_assoc
branch
from
4ca1bc3 to
bf2af28
Compare
|
update:
|
| # If the association were already translated, then short circuit / do the standard preloader work. | ||
| # When replace_virtual_fields removes the outer array, match that too. | ||
| if records_by_assoc.size == 1 && | ||
| (associations == records_by_assoc.keys.first || associations == [records_by_assoc.keys.first]) |
There was a problem hiding this comment.
Why is it sometimes wrapped in an array and not other times?
There was a problem hiding this comment.
Would flatten or similar help keep it consistent for comparison?
There was a problem hiding this comment.
The associaions will probably always be an array. Even a hash becomes a single element array with a hash in it.
fix_association optimizes the array right out of there.
So we end up comparing :symbol with [:symbol], they are different and we get one extra set of lambdas (and one extra group_by) - all with the same class.
This is an optimization only, Without this change, all works fine.
There was a problem hiding this comment.
Right, would Array.wrap(...) normalize to an array make this less complex or introduce a possible performance hit for the case when it's already an array?
There was a problem hiding this comment.
I had thought I could avoid this via:
- assoc_cache = Hash.new { |h, klass| h[klass] = klass.replace_virtual_fields(associations) }
+ assoc_cache = Hash.new { |h, klass| h[klass] = Array.wrap(klass.replace_virtual_fields(associations)) }Since we have Array.wrap on that field later in the block, but ended up with an infinite loop.
So if we do this, I'll need to do the reverse:
records_by_assoc.keys.first == [associations] || records_by_assoc.keys.first == associationsStill tempting
spec/virtual_includes_spec.rb
Outdated
| it "preloads standard associations (:books)" do | ||
| expect(Author.eager_load(:books)).to preload_values(:first_book_name, book_name) | ||
| expect(Author.eager_load([:books])).to preload_values(:first_book_name, book_name) | ||
| expect(Author.eager_load([[:books]])).to preload_values(:first_book_name, book_name) | ||
| expect(Author.eager_load(:books => {})).to preload_values(:first_book_name, book_name) | ||
| end |
There was a problem hiding this comment.
This test looks like a straight duplicate of the one at the beginning of this describe.
There was a problem hiding this comment.
We should probably be using eager_load/left_join or preload/includes and get away from the whole includes().references().
I added a section for eager_load and preload, which are basic mechanisms and just ensuring that they are properly translating virtual attributes and handling the various forms. So these will look like pure duplication - but using different methods.
There was a problem hiding this comment.
No, I mean it's a literal copy paste of the set of test just 10 lines up
kbrock
force-pushed
the
preload_array_assoc
branch
from
bf2af28 to
17cb22d
Compare
spec/virtual_includes_spec.rb
Outdated
| expect(Author.preload(:first_book_name => {})).to preload_values(:first_book_name, book_name) | ||
| end | ||
|
|
||
| it "preloads standard associations (:books)" do |
There was a problem hiding this comment.
This one is also a straight duplicate of the set of tests just 10 lines up.
This ends up being an optimization. But when preloading `[:association]`, it runs a separate preloader on `:association`. This skips the interim step
kbrock
force-pushed
the
preload_array_assoc
branch
from
17cb22d to
a37b1df
Compare
|
Checked commit kbrock@a37b1df with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
1 similar comment
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
This ends up being an optimization.
But when preloading
[:association], it runs a separate preloader on:association. This skips the interim step.I was seeing an infinite loop, but at this time, this is only a minor difference.
I am pushing this in because I would like to avoid running the group_by multiple times.