Skip to content

HHH-14097 Fix redundant SQLs issue for fetch entity graph #3453

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

Merged
merged 1 commit into from
Jul 9, 2020
Merged

HHH-14097 Fix redundant SQLs issue for fetch entity graph #3453

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
9 changes: 8 additions & 1 deletion documentation/src/main/asciidoc/userguide/chapters/fetching/Fetching.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,15 @@ In both cases, this resolves to exactly one database query to get all that infor
[[fetching-strategies-dynamic-fetching-entity-graph]]
=== Dynamic fetching via JPA entity graph

JPA 2.1 introduced entity graphs so the application developer has more control over fetch plans.
JPA 2.1 introduced ``entity graph`` so the application developer has more control over fetch plans. It has two modes to choose from:

fetch graph:::
In this case, all attributes specified in the entity graph will be treated as FetchType.EAGER, and all attributes not specified will *ALWAYS* be treated as FetchType.LAZY.

load graph:::
In this case, all attributes specified in the entity graph will be treated as FetchType.EAGER, but attributes not specified use their static mapping specification.

Below is an `fetch graph` dynamic fetching example:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above content was copied from v6's counterpart. Thought the basic explanation of the two types of entity graph modes is supposed to exist in v5 as well.
Given the content has existed in v6 already, we don't need to bother merging this part to v6 as well.

[[fetching-strategies-dynamic-fetching-entity-graph-example]]
.Fetch graph example
====
Expand Down
119 changes: 74 additions & 45 deletions hibernate-core/src/main/java/org/hibernate/engine/internal/TwoPhaseLoad.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,62 +206,91 @@ public static void initializeEntityEntryLoadedState(
String entityName = persister.getEntityName();
String[] propertyNames = persister.getPropertyNames();
final Type[] types = persister.getPropertyTypes();

final GraphImplementor<?> fetchGraphContext = session.getFetchGraphLoadContext();

for ( int i = 0; i < hydratedState.length; i++ ) {
final Object value = hydratedState[i];
if ( debugEnabled ) {
LOG.debugf(
"Processing attribute `%s` : value = %s",
propertyNames[i],
value == LazyPropertyInitializer.UNFETCHED_PROPERTY ? "<un-fetched>" : value == PropertyAccessStrategyBackRefImpl.UNKNOWN ? "<unknown>" : value
);
}

if ( value == LazyPropertyInitializer.UNFETCHED_PROPERTY ) {
GraphImplementor fetchGraphContext = session.getFetchGraphLoadContext();
if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
fetchGraphContext = null;
session.setFetchGraphLoadContext( null );
Copy link
Member

Choose a reason for hiding this comment

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

just a minor but I think the session.setFetchGraphLoadContext( null ); is redundant

Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is necessary because without setting it, the downstream will still try to locate some property as per the context grabbed from session.getFetchGraphLoadContext(), not from the local variable of fetchGraphContext. See the org.hibernate.engine.internal.TwoPhaseLoad#isEagerFetchGraph(SharedSessionContractImplementor session, String associationName, Type associationType) for details.

I am sorry the current fetch entity graph implementation is so difficult to understand (and maintain). Luckily its status is improved much more in v6.

}

try {
for ( int i = 0; i < hydratedState.length; i++ ) {
final Object value = hydratedState[i];
if ( debugEnabled ) {
LOG.debugf( "Resolving <un-fetched> attribute : `%s`", propertyNames[i] );
LOG.debugf(
"Processing attribute `%s` : value = %s",
propertyNames[i],
value == LazyPropertyInitializer.UNFETCHED_PROPERTY ?
"<un-fetched>" :
value == PropertyAccessStrategyBackRefImpl.UNKNOWN ? "<unknown>" : value
);
}

// IMPLEMENTATION NOTE: This is a lazy property on a bytecode-enhanced entity.
// hydratedState[i] needs to remain LazyPropertyInitializer.UNFETCHED_PROPERTY so that
// setPropertyValues() below (ultimately AbstractEntityTuplizer#setPropertyValues) works properly
// No resolution is necessary, unless the lazy property is a collection.
if ( types[i].isCollectionType() ) {
// IMPLEMENTATION NOTE: this is a lazy collection property on a bytecode-enhanced entity.
// HHH-10989: We need to resolve the collection so that a CollectionReference is added to StatefulPersistentContext.
// As mentioned above, hydratedState[i] needs to remain LazyPropertyInitializer.UNFETCHED_PROPERTY
// so do not assign the resolved, uninitialized PersistentCollection back to hydratedState[i].
Boolean overridingEager = getOverridingEager( session, entityName, propertyNames[i], types[i], debugEnabled );
types[i].resolve( value, session, entity, overridingEager );
}
}
else if ( value != PropertyAccessStrategyBackRefImpl.UNKNOWN ) {
if ( debugEnabled ) {
final boolean isLazyEnhanced = persister.getBytecodeEnhancementMetadata()
.getLazyAttributesMetadata()
.getLazyAttributeNames()
.contains( propertyNames[i] );
LOG.debugf( "Attribute (`%s`) - enhanced for lazy-loading? - %s", propertyNames[i], isLazyEnhanced );
if ( value == LazyPropertyInitializer.UNFETCHED_PROPERTY ) {
if ( debugEnabled ) {
LOG.debugf( "Resolving <un-fetched> attribute : `%s`", propertyNames[i] );
}

// IMPLEMENTATION NOTE: This is a lazy property on a bytecode-enhanced entity.
// hydratedState[i] needs to remain LazyPropertyInitializer.UNFETCHED_PROPERTY so that
// setPropertyValues() below (ultimately AbstractEntityTuplizer#setPropertyValues) works properly
// No resolution is necessary, unless the lazy property is a collection.
if ( types[i].isCollectionType() ) {
// IMPLEMENTATION NOTE: this is a lazy collection property on a bytecode-enhanced entity.
// HHH-10989: We need to resolve the collection so that a CollectionReference is added to StatefulPersistentContext.
// As mentioned above, hydratedState[i] needs to remain LazyPropertyInitializer.UNFETCHED_PROPERTY
// so do not assign the resolved, uninitialized PersistentCollection back to hydratedState[i].
Boolean overridingEager = getOverridingEager(
session,
entityName,
propertyNames[i],
types[i],
debugEnabled
);
types[i].resolve( value, session, entity, overridingEager );
}
}
else if ( value != PropertyAccessStrategyBackRefImpl.UNKNOWN ) {
if ( debugEnabled ) {
final boolean isLazyEnhanced = persister.getBytecodeEnhancementMetadata()
.getLazyAttributesMetadata()
.getLazyAttributeNames()
.contains( propertyNames[i] );
LOG.debugf(
"Attribute (`%s`) - enhanced for lazy-loading? - %s",
propertyNames[i],
isLazyEnhanced
);
}

// we know value != LazyPropertyInitializer.UNFETCHED_PROPERTY
Boolean overridingEager = getOverridingEager( session, entityName, propertyNames[i], types[i], debugEnabled );
hydratedState[i] = types[i].isEntityType()
? entityResolver.resolve( (EntityType) types[i], value, session, entity, overridingEager )
: types[i].resolve( value, session, entity, overridingEager );
}
else {
if ( debugEnabled ) {
LOG.debugf( "Skipping <unknown> attribute : `%s`", propertyNames[i] );
// we know value != LazyPropertyInitializer.UNFETCHED_PROPERTY
Boolean overridingEager = getOverridingEager(
session,
entityName,
propertyNames[i],
types[i],
debugEnabled
);
hydratedState[i] = types[i].isEntityType()
? entityResolver.resolve( (EntityType) types[i], value, session, entity, overridingEager )
: types[i].resolve( value, session, entity, overridingEager );
}
else {
if ( debugEnabled ) {
LOG.debugf( "Skipping <unknown> attribute : `%s`", propertyNames[i] );
}
}
}

if ( session.getFetchGraphLoadContext() != fetchGraphContext ) {
session.setFetchGraphLoadContext( fetchGraphContext );
}
}
finally {
// HHH-14097
// Fetch entity graph should be applied only once on top level (for root hydrated object)
// e.g., see org.hibernate.loader.Loader for details
session.setFetchGraphLoadContext( null );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this, are we entirely sure that the fetch graph is no longer needed at this point?
Wouldn't it be possible for the fetch grapth to need to span multiple load events?

IF this is correct, should line 285 be removed as it seems redundant?

Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For non-toplevel traversal, Line #285 will take effect to backtrack to upper level after the above statement. The above statement will impact only top level (I made it explicitly by including "on top level" in the comments above). I admit the code change is difficult to understand for basically 'fetch graph' implementing is sort of based on hack (existing v5 infrastructure makes it difficult to implement elegantly), but v6 makes the implementation much easier and more elegant (it has well-designed context info accessible). After this PR gets merged, I'll port it into v6 to see whether the issue still remains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubled checked and found the backtracking is working as expected (without it our graph testing cases would fail). Could you take a new look at this PR? Thanks.

}
}

public static void initializeEntityFromEntityEntryLoadedState(
Expand Down
Loading