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

Add support for direct_import parameter to skip loading default scope on import #637

Closed
wants to merge 3 commits into from

Conversation

TikiTDO
Copy link
Contributor

@TikiTDO TikiTDO commented Feb 23, 2018

Proof of Concept to get around issue outlined in #636.

Currently there's no tests, as I wasn't sure how the spec files were structured for this project. I can add the specs if someone could point me to an example where I could exercise the functionality of importing objects with extra includes.

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Feb 25, 2018

@pyromaniac Does this implementation work for you?

@abraham-chan
Copy link

@TikiTDO I'm no maintainer but I see some import specs for active_record here:
https://github.com/toptal/chewy/blob/master/spec/chewy/type/adapter/active_record_spec.rb#L68

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Apr 20, 2018

@abraham-chan I've moved quite a far from this section of my project, and I've got work queued up for months ahead. If you wish to take over, then feel free to fork this branch, otherwise I might at best have time to look at this in a few months

@jeremyhaile
Copy link

I was amazed to realize that if you call import with an array of ActiveRecord objects, it reruns a query to get the same ActiveRecord objects and then uses those. Which means if you included associations it forces an N+1 query.

Can anyone (maybe @pyromaniac ?) explain why it re-queries for objects that are already in memory prior to sending to ES?

Is there any workarounds, or would this branch be needed in order to import AR objects that are already in memory without reloading them from the DB?

@toptal-bot
Copy link
Member

Can one of the admins verify this patch?

dtpowl added a commit to sideqik/chewy that referenced this pull request Jul 5, 2019
@pyromaniac
Copy link
Contributor

Well, I did it because it looks more reliable. I can merge this option as soon as some specs will be added.

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Jul 6, 2019

Looking at this I vaguely remember that this had something to do with a manual importer, but I can't actually recall the nature of the problem anymore.

@dtpowl you referenced this recently, is this something you could take over, or failing that could you remind me what issue this was trying to solve?

@dalthon
Copy link
Contributor

dalthon commented Feb 9, 2021

@TikiTDO thanks for you contribution!
Your PR was rebased at #753 and your added option was covered with spec, soon we will merge that rebased PR

@dalthon dalthon closed this Feb 9, 2021
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.

6 participants