Skip to content

NH-3675 - Support Bulk Inserts #367

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

NH-3675 - Support Bulk Inserts #367

wants to merge 3 commits into from

Conversation

rjperes
Copy link
Member

@rjperes rjperes commented Oct 29, 2014

https://nhibernate.jira.com/browse/NH-3675
Initial implementation with unit tests

{
foreach (var entity in entities)
{
if (session is ISession)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this code!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why is that? Another option is to return null, that is, not have a default bulk provider

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it uses "is-as" checks. A preffereble way is to use "as" and compare to null. Mainly because "is" and "as" are compiled to the same IL opcode "isinst".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that is totally irrelevant. But I changed this method to always use stateless session, don't want to store these entities in the first level cache

@hazzik
Copy link
Member

hazzik commented Oct 29, 2014

Hi @rjperes, thanks for the great work. But can you please utilize IEntityPersister.GetPropertyValuesToInsert to populate DataTable instead of DataBinder?

@rjperes
Copy link
Member Author

rjperes commented Oct 29, 2014

Can't use IEntityPersister.GetPropertyValuesToInsert because it returns an array of objects, containing possibly complex classes, in the case of components, many-to-one or one-to-one associations.

@hazzik
Copy link
Member

hazzik commented Oct 29, 2014

2014-10-29 21:15 GMT+13:00 Ricardo Peres [email protected]:

Can't use IEntityPersister.GetPropertyValuesToInsert because it returns an
array of objects, containing possibly complex classes, in the case of
components, many-to-one or one-to-one associations.

We eitther need (1)to teach IType to set values to a DataTable, instead of
IDbCommand; or (2) use IType.Disassembly and IType.Resolve

var persister = session.GetEntityPersister(entityType.FullName, null) as AbstractEntityPersister;
var table = new DataTable();

if (persister is SingleTableEntityPersister)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following if-then-else can be replaced with only one

var abstractEntityPersister = persister as AbstractEntityPersister;
if (abstractEntityPersister != null) {
    table.TableName = abstractEntityPersister .TableName;
}

Because SingleTableEntityPersister, JoinedSubclassEntityPersister, and UnionSubclassEntityPersister inherit AbstractEntityPersister and TableName is it's property

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, persister is already AbstractEntityPersister, so just persister.TableName;

@hazzik hazzik changed the title NH-3675 NH-3675 - Support Bulk Inserts Nov 18, 2014
@hazzik hazzik added this to the 5.0.0 milestone Nov 18, 2014
@hazzik hazzik modified the milestones: 6.0, 5.0 Aug 3, 2017
Added DefaultBulkProvider class as the default
Removed dependency of System.Web and DataBinder.Eval
Using ADOExceptionHelper.Convert to convert DB exceptions
BulkInsert only applies to IStatelessSession
Small improvements
@micmerchant
Copy link
Contributor

micmerchant commented Feb 13, 2019

Hello, I stumbled over this pull request today and would appreciate if this feature would be accepted. We've also integrated a bulk implementation for MSSQL done in the same way in our product and I would consider to replace it by native NHibernate functionality in this case.

However, I got some issues:

  • What about insert listeners? If there are some specific listeners targeting the entity type, shouldn't they be called in case? An additional argument could be added e.g. bool callListeners
  • What about using ISession over IStatelessSession and adding an additional parameter e.g. bool attachToSession?
  • What about logging? It would be useful to log at least the ids of the entities to be inserted, in order to track it via the NHibernateProfiler.
  • I see the table configuration as a possible performance issue, because it's done with every insert call. What if a hash map having the entity type as key would be incrementally filled? Then you would be able to retrieve the table configuration using the dictionary. Of course you would have to call DataTable::Clear after every call.

@fredericDelaporte
Copy link
Member

The PR opener having deleted his nhibernate repository, it seems likely to me this PR will not move forward unless someone re-issue it and work on it. For how to do that with a deleted repository, one way is to do like here.

@micmerchant
Copy link
Contributor

What a pity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants