Skip to content

HHH-7572 - Develop API for load-by-multiple-ids #1136

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

HHH-7572 - Develop API for load-by-multiple-ids #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions hibernate-core/src/main/java/org/hibernate/IdentifierLoadAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
package org.hibernate;

import java.io.Serializable;
import java.util.List;

/**
* Loads an entity by its primary identifier.
* Loads an entity (or multiple) by its primary identifier.
*
* @author Eric Dalquist
* @author Steve Ebersole
Expand All @@ -22,7 +23,16 @@ public interface IdentifierLoadAccess<T> {
*
* @return {@code this}, for method chaining
*/
public IdentifierLoadAccess<T> with(LockOptions lockOptions);
IdentifierLoadAccess<T> with(LockOptions lockOptions);

/**
* Specify the {@link CacheMode} to use when retrieving the entity.
*
* @param cacheMode The CacheMode to use.
*
* @return {@code this}, for method chaining
*/
IdentifierLoadAccess<T> with(CacheMode cacheMode);

/**
* Return the persistent instance with the given identifier, assuming that the instance exists. This method
Expand All @@ -36,7 +46,7 @@ public interface IdentifierLoadAccess<T> {
*
* @return the persistent instance or proxy
*/
public T getReference(Serializable id);
T getReference(Serializable id);

/**
* Return the persistent instance with the given identifier, or null if there is no such persistent instance.
Expand All @@ -47,5 +57,25 @@ public interface IdentifierLoadAccess<T> {
*
* @return The persistent instance or {@code null}
*/
public T load(Serializable id);
T load(Serializable id);

/**
* Perform a load of multiple entities by identifiers
*
* @param ids The ids to load
* @param <K> The identifier type
*
* @return The persistent entities.
*/
<K extends Serializable> List<T> multiLoad(K... ids);

/**
* Perform a load of multiple entities by identifiers
*
* @param ids The ids to load
* @param <K> The identifier type
*
* @return The persistent entities.
*/
<K extends Serializable> List<T> multiLoad(List<K> ids);
}
12 changes: 12 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import org.hibernate.internal.util.StringHelper;
import org.hibernate.internal.util.collections.ArrayHelper;
import org.hibernate.internal.util.io.StreamCopier;
import org.hibernate.loader.BatchLoadSizingStrategy;
import org.hibernate.mapping.Column;
import org.hibernate.mapping.Constraint;
import org.hibernate.mapping.ForeignKey;
Expand Down Expand Up @@ -2813,4 +2814,15 @@ public CallableStatementSupport getCallableStatementSupport() {
public NameQualifierSupport getNameQualifierSupport() {
return null;
}

protected final BatchLoadSizingStrategy STANDARD_DEFAULT_BATCH_LOAD_SIZING_STRATEGY = new BatchLoadSizingStrategy() {
@Override
public int determineOptimalBatchLoadSize(int numberOfKeyColumns, int numberOfKeys) {
return 50;
}
};

public BatchLoadSizingStrategy getDefaultBatchLoadSizingStrategy() {
return STANDARD_DEFAULT_BATCH_LOAD_SIZING_STRATEGY;
}
}
118 changes: 116 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.sql.Connection;
import java.sql.NClob;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
Expand Down Expand Up @@ -129,6 +130,8 @@
import org.hibernate.loader.criteria.CriteriaLoader;
import org.hibernate.loader.custom.CustomLoader;
import org.hibernate.loader.custom.CustomQuery;
import org.hibernate.loader.entity.DynamicBatchingEntityLoaderBuilder;
import org.hibernate.loader.entity.DynamicBatchingEntityLoaderBuilder.DynamicBatchingEntityLoader;
import org.hibernate.persister.collection.CollectionPersister;
import org.hibernate.persister.entity.EntityPersister;
import org.hibernate.persister.entity.OuterJoinLoadable;
Expand Down Expand Up @@ -2577,6 +2580,7 @@ public void lock(Object object) throws HibernateException {
private class IdentifierLoadAccessImpl<T> implements IdentifierLoadAccess<T> {
private final EntityPersister entityPersister;
private LockOptions lockOptions;
private CacheMode cacheMode;

private IdentifierLoadAccessImpl(EntityPersister entityPersister) {
this.entityPersister = entityPersister;
Expand All @@ -2597,8 +2601,37 @@ public final IdentifierLoadAccessImpl<T> with(LockOptions lockOptions) {
}

@Override
@SuppressWarnings("unchecked")
public IdentifierLoadAccess<T> with(CacheMode cacheMode) {
this.cacheMode = cacheMode;
return this;
}

@Override
public final T getReference(Serializable id) {
CacheMode sessionCacheMode = getCacheMode();
boolean cacheModeChanged = false;
if ( cacheMode != null ) {
// naive check for now...
// todo : account for "conceptually equal"
if ( cacheMode != sessionCacheMode ) {
setCacheMode( cacheMode );
cacheModeChanged = true;
}
}

try {
return doGetReference( id );
}
finally {
if ( cacheModeChanged ) {
// change it back
setCacheMode( sessionCacheMode );
}
}
}

@SuppressWarnings("unchecked")
private T doGetReference(Serializable id) {
if ( this.lockOptions != null ) {
LoadEvent event = new LoadEvent( id, entityPersister.getEntityName(), lockOptions, SessionImpl.this );
fireLoad( event, LoadEventListener.LOAD );
Expand All @@ -2624,8 +2657,31 @@ public final T getReference(Serializable id) {
}

@Override
@SuppressWarnings("unchecked")
public final T load(Serializable id) {
CacheMode sessionCacheMode = getCacheMode();
boolean cacheModeChanged = false;
if ( cacheMode != null ) {
// naive check for now...
// todo : account for "conceptually equal"
if ( cacheMode != sessionCacheMode ) {
setCacheMode( cacheMode );
cacheModeChanged = true;
}
}

try {
return doLoad( id );
}
finally {
if ( cacheModeChanged ) {
// change it back
setCacheMode( sessionCacheMode );
}
}
}

@SuppressWarnings("unchecked")
public final T doLoad(Serializable id) {
if ( this.lockOptions != null ) {
LoadEvent event = new LoadEvent( id, entityPersister.getEntityName(), lockOptions, SessionImpl.this );
fireLoad( event, LoadEventListener.GET );
Expand All @@ -2646,6 +2702,64 @@ public final T load(Serializable id) {
}
return (T) event.getResult();
}

@Override
public <K extends Serializable> List<T> multiLoad(K... ids) {
CacheMode sessionCacheMode = getCacheMode();
boolean cacheModeChanged = false;
if ( cacheMode != null ) {
// naive check for now...
// todo : account for "conceptually equal"
if ( cacheMode != sessionCacheMode ) {
setCacheMode( cacheMode );
cacheModeChanged = true;
}
}

try {
return DynamicBatchingEntityLoaderBuilder.INSTANCE.multiLoad(
(OuterJoinLoadable) entityPersister,
ids,
lockOptions,
SessionImpl.this
);
}
finally {
if ( cacheModeChanged ) {
// change it back
setCacheMode( sessionCacheMode );
}
}
}

@Override
public <K extends Serializable> List<T> multiLoad(List<K> ids) {
CacheMode sessionCacheMode = getCacheMode();
boolean cacheModeChanged = false;
if ( cacheMode != null ) {
// naive check for now...
// todo : account for "conceptually equal"
if ( cacheMode != sessionCacheMode ) {
setCacheMode( cacheMode );
cacheModeChanged = true;
}
}

try {
return DynamicBatchingEntityLoaderBuilder.INSTANCE.multiLoad(
(OuterJoinLoadable) entityPersister,
ids.toArray( new Serializable[ ids.size() ] ),
lockOptions,
SessionImpl.this
);
}
finally {
if ( cacheModeChanged ) {
// change it back
setCacheMode( sessionCacheMode );
}
}
}
}

private EntityPersister locateEntityPersister(Class entityClass) {
Expand Down
16 changes: 16 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/loader/BatchLoadSizingStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later.
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.loader;

/**
* Strategy (pluggable) for determining an optimal size for batch loads.
*
* @author Steve Ebersole
*/
public interface BatchLoadSizingStrategy {
int determineOptimalBatchLoadSize(int numberOfKeyColumns, int numberOfKeys);
Copy link
Member

Choose a reason for hiding this comment

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

Why is numberOfKeyColumns part of the contract? Would be nice to add a JavaDoc comment describing how implementors are meant to utilize it.

Also should be the entity type of interest be passed as well? One might want to load small master data table in different chunks than a bug transaction data table for instance.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, looking further it seems dialect authors are the intended implementors of this contract, not users as I first thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its not part of the JavaDoc because as I stated in the PR description this is intended just for us to discuss. Do you typically fill out JavaDoc for such prototyping?

Well numberOfKeyColumns is important to be able to understand how many parameters this translates to in the SQL call for databases that limit the numer of parameters. Loading 1000 entities whose id is made up of 1 column requires 1000 parameters, but loading 1000 entities whose id is made up of 2 column requires 2000 parameters... Big difference.

The idea of this is for the Dialect to be able to influence the batch size when we were not told a size by the user as discussed in the Jira.

Look, this is a prototype meant for us to discuss the design. So if you have something you'd like to see different that is the point of this. But really constructive criticism is usually better. So maybe rather than just pointing out what you don't like you could propose an alternative? But in terms of this contract you are just missing the point of it (as it was discussed in the Jira).

As for the user being able to tell us the batch size, the discussion for that (again, on the Jira) was to use @BatchSize (which is entity specific). Again, if you don't like that then propose a way for that to happen. However I will say that overloading these methods to accept a "batchSize" is not something I would vote for; thats classic implementation details leaking through an API.

Copy link
Member

Choose a reason for hiding this comment

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

Do you typically fill out JavaDoc for such prototyping?

Yes, I do it where I think it helps reviewers to better understand my draft.

Well numberOfKeyColumns is important to be able...

Got it, thanks.

So if you have something you'd like to see different that is the point of this. But really constructive criticism is usually better. So maybe rather than just pointing out what you don't like you could propose an alternative?

Where did I express any dislike? I asked one question for understanding, followed by a proposal how others could be prevented from asking the same question. I find that very constructive.

Then I made another suggestion (in form of a question). Granted, that suggestion was based on a wrong understanding of the API's purpose - which I acknowledged in my second comment.

I don't see how this can be conceived as not constructive, I am sorry if you got that impression.

Anyways, the contract looks good in this light. Introducing a parameter object may be a good idea with respect to evolution of the API (hint: this is a proposal for an alternative ;).

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, I do it where I think it helps reviewers to better understand my draft.

When the same points are already discussed in the Jira? Ok, bonus points to you I guess...

Introducing a parameter object may be a good idea with respect to evolution of the API (hint: this is a proposal for an alternative ;)

If we were to go that route, I'd actually just move these from IdentifierLoadAccess into a new contract (MultIdentifierLoadAccess?) Ultimately it is the same principle, the difference being that the user isn't having to instantiate that extra object directly.

}
63 changes: 63 additions & 0 deletions ...te-core/src/main/java/org/hibernate/loader/entity/DynamicBatchingEntityLoaderBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.hibernate.LockMode;
Expand All @@ -24,9 +25,11 @@
import org.hibernate.engine.spi.SessionImplementor;
import org.hibernate.internal.util.StringHelper;
import org.hibernate.internal.util.collections.ArrayHelper;
import org.hibernate.internal.util.collections.CollectionHelper;
import org.hibernate.loader.spi.AfterLoadAction;
import org.hibernate.persister.entity.OuterJoinLoadable;
import org.hibernate.pretty.MessageHelper;
import org.hibernate.type.Type;

import org.jboss.logging.Logger;

Expand All @@ -41,6 +44,66 @@ public class DynamicBatchingEntityLoaderBuilder extends BatchingEntityLoaderBuil

public static final DynamicBatchingEntityLoaderBuilder INSTANCE = new DynamicBatchingEntityLoaderBuilder();

@SuppressWarnings("unchecked")
public <T, K extends Serializable> List<T> multiLoad(
OuterJoinLoadable persister,
K[] ids,
LockOptions lockOptions,
SessionImplementor session) {
List<T> result = CollectionHelper.arrayList( ids.length );

if ( lockOptions == null ) {
lockOptions = new LockOptions( LockMode.NONE );
}

int numberOfIdsLeft = ids.length;
int optimalMaxBatchSize = session.getFactory().getDialect().getDefaultBatchLoadSizingStrategy().determineOptimalBatchLoadSize(
persister.getIdentifierType().getColumnSpan( session.getFactory() ),
numberOfIdsLeft
);

int idPosition = 0;
while ( numberOfIdsLeft > 0 ) {
int batchSize = Math.min( numberOfIdsLeft, optimalMaxBatchSize );
final DynamicEntityLoader batchingLoader = new DynamicEntityLoader(
persister,
batchSize,
lockOptions,
session.getFactory(),
session.getLoadQueryInfluencers()
);

Serializable[] idsInBatch = new Serializable[batchSize];
System.arraycopy( ids, idPosition, idsInBatch, 0, batchSize );

QueryParameters qp = buildMultiLoadQueryParameters( persister, idsInBatch, lockOptions );
result.addAll( batchingLoader.doEntityBatchFetch( session, qp, idsInBatch ) );

numberOfIdsLeft = numberOfIdsLeft - batchSize;
idPosition += batchSize;
}

return result;
}

public static QueryParameters buildMultiLoadQueryParameters(
OuterJoinLoadable persister,
Serializable[] ids,
LockOptions lockOptions) {
Type[] types = new Type[ids.length];
Arrays.fill( types, persister.getIdentifierType() );

QueryParameters qp = new QueryParameters();
qp.setOptionalEntityName( persister.getEntityName() );
qp.setPositionalParameterTypes( types );
qp.setPositionalParameterValues( ids );
qp.setLockOptions( lockOptions );
qp.setOptionalObject( null );
qp.setOptionalId( null );
return qp;
}


@Override
protected UniqueEntityLoader buildBatchingLoader(
OuterJoinLoadable persister,
Expand Down
Loading