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

Conversation

Projects
None yet
2 participants
@wubzz
Copy link
Collaborator

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);

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

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.


return knex.batchInsert('BatchInsert', items, 1);
})
.then(function() {

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

returning promise is a bit nicer here instead of calling done() also here one could check that correct number of lines were inserted.

batch = _.chunk(batch, chunkSize)
}

Promise.all(batch.map((items) => {

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

returning promise from transaction should call trx.commit / trx.rollback automatically if I recall correctly. In that way one doesnt need to do explicit callst to tr.commit / tr.rollback

@elhigu

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

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 pull request Feb 4, 2016

@elhigu elhigu merged commit 8d75d3d into tgriesser:master Feb 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@indeyets indeyets referenced this pull request Feb 27, 2016

Merged

Fix/geonames importer #139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.