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

adapter.createEach not called #5501

Closed
atiertant opened this issue May 14, 2015 · 13 comments
Closed

adapter.createEach not called #5501

atiertant opened this issue May 14, 2015 · 13 comments

Comments

@atiertant
Copy link

adapter.createEach is only called in alter mode to reinsert data.
shouldn't it optimize insertion of multiple data ?

@dmarcelino
Copy link
Member

Yes, it should be called when calling .create([], cb);. This looks like a bug.

@devinivy
Copy link

I haven't tested, but I'm afraid createEach is possibly never optimized.

See Collection#createEach always passing its work onto Collection#create: https://github.com/balderdashy/waterline/blob/fe402789bd89d343c3f673c95e01415a65ce8176/lib/waterline/query/aggregate.js#L24

On the other hand, the adapter createEach method is ready to optimize, but I don't think the adapter method is ever used: https://github.com/balderdashy/waterline/blob/fe402789bd89d343c3f673c95e01415a65ce8176/lib/waterline/adapter/aggregateQueries.js#L13

It appears Collection#create with an array does ask Collection#createEach to do the work, so this issue probably appears when using either syntax: https://github.com/balderdashy/waterline/blob/fe402789bd89d343c3f673c95e01415a65ce8176/lib/waterline/query/dql/create.js#L60-L63

If this is indeed a bug, we'll have to do some work to hook the lifecycle into createEach when it's optimized by an adapter.

@dmarcelino
Copy link
Member

I haven't tested, but I'm afraid createEach is possibly never optimised.

Back in balderdashy/waterline#980 (comment) I did notice sails-postgresql and sails-mysql were using waterline's .createEach() shim, even though they define their own .createEach() in adapter.js#L402 and adapter.js#L424 respectively.

If this is indeed a bug,

I think it is.

we'll have to do some work to hook the lifecycle into createEach when it's optimized by an adapter.

Agreed.

@masumsoft
Copy link

Same here. It is causing really slow write performance as batch writes are not using adapter's createEach() method, rather sending individual create queries and thus insert performance is drastically degraded.

As a clue, the findOrCreateEach() method is properly calling adapter's method if adapter implemented one.

masumsoft referenced this issue in masumsoft/waterline May 21, 2015
Sending createEach values to adapter's optimized .createEach() method
Will fallback to normal create if adapter doesn't implement .createEach()
@dmarcelino
Copy link
Member

Hi @masumsoft, I've left some comments (balderdashy/waterline#1018 (comment)) on PR #1018. Please take a look.

@masumsoft
Copy link

Hi @dmarcelino, Thanks for the nice feedback. I was a bit busy this week, didn't have much time to look at it, will try to resolve the issues mentioned by you asap.

@masumsoft
Copy link

Updated the PR #1018 with new commits, details available there.

@sailsbot
Copy link

Thanks for posting, @atiertant. I'm a repo bot-- nice to meet you!

It has been 60 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@devinivy
Copy link

This has a PR in the works.

@devinivy devinivy reopened this Sep 22, 2015
@particlebanana
Copy link
Contributor

The main issue with this is handling nested creates. That's why it currently loops through and runs .create for each item in the array. So each item needs to essentially go through this flow:

• Set model default values
• Create any nested records that belong to the parent
• Run any beforeCreate lifecycle callbacks that have been defined
• Create the record
• Associate any nested m:m records with the newly created record
• Run any afterCreate callbacks that have been defined
• Turn object into a Waterline model

If we were to pass that whole array to the adapter's createEach method we would need to flatten out the nested records and handle the logic for each nested collection item. Which I don't think you could get in a single insert query anyway because they would be inserting records into multiple tables.

Now we could simplify this and say you can't do nested creates in a createEach which would allow this to be much simpler. Then you just loop through the array running beforeCreate and adding default values then pass the array to the adapter.

@atiertant
Copy link
Author

@particlebanana an other way would be to group flat insert by model so an array of two level nested create could be insert in two createEach like deepPopulate but with nested create logic.

@sailsbot
Copy link

sailsbot commented Nov 3, 2015

Thanks for posting, @atiertant. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@sailsbot sailsbot closed this as completed Nov 3, 2015
@devinivy devinivy reopened this Nov 3, 2015
@particlebanana
Copy link
Contributor

This is on the roadmap file. Closing it now. To clarify there needs to be a proposal as to how this change effects all the various nested updates operations.

@raqem raqem transferred this issue from balderdashy/waterline May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants