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

knex.batchInsert #1182

Merged
merged 7 commits into from Feb 4, 2016
Merged

knex.batchInsert #1182

merged 7 commits into from Feb 4, 2016

Conversation

@wubzz
Copy link
Member

@wubzz wubzz commented Feb 2, 2016

This was discussed in #757. Couldn't see Tim's code in that issue so not sure how he originally implemented this, but hopefully I'm not too far of.

Ideas for performance improvements is more than welcome. It still takes a bit to insert 5k+ rows.

In short it will create a transaction and insert all the rows within that transaction.

knex.batchInsert(tableName, rows, chunkSize = 1)
@@ -647,6 +647,37 @@ module.exports = function(knex) {
});
});


it('#757 - knex.batchInsert(tableName, bulk, chunkSize)', function (done) {
this.timeout(20000);
Copy link
Member

@elhigu elhigu Feb 3, 2016

Just to make sure that batch insert does what it is supposed to dataset could be e.g. 100 items, which are added in batch of 30 items or so? We need to avoid long running tests.

@elhigu
Copy link
Member

@elhigu elhigu commented Feb 3, 2016

@wubzz thaks for PR, I left few comments on the code. Also documentation should be added for the new feature. The long running test was the only serious issue in the code. Other stuff are more just opinions :)

@wubzz
Copy link
Member Author

@wubzz wubzz commented Feb 4, 2016

@elhigu Thanks for the feedback. I've updated the test and added info to the docs. There was no obvious section in the docs to place this in so I added a 'Utility' section. Let me know if I need to change it! :)

elhigu added a commit that referenced this issue Feb 4, 2016
@elhigu elhigu merged commit 8d75d3d into knex:master Feb 4, 2016
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants