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

NathanQingyangXu · 2020-07-04T11:26:15Z

https://hibernate.atlassian.net/browse/HHH-14097

The root cause is we lack fetch entity graph validation. For the initial multiple hydrated objects, only the first one is applicable to root entity graph.

Imported andrea's testing code from #3452. Tons of thanks to him!

Remember only the testing case is needed to be merged into v6 for entity graph implementation is incompatible between v5 and v6 and the code changes except the testing case are not needed in v6 (actually I've carefully deleted the v5 implementation in v6 previously).

documentation/src/test/java/org/hibernate/userguide/fetching/GraphFetchingTest.java

Sanne · 2020-07-06T14:55:12Z

hibernate-core/src/main/java/org/hibernate/engine/internal/TwoPhaseLoad.java

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?

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.

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.

dreab8 · 2020-07-06T15:22:14Z

Hi @NathanQingyangXu still not sure we really need session.setFetchGraphLoadContext( null ); as discussed privately I tried to add the kind of test you think may need this call

@Test
	public void testQueryByIdWithLoadGraph3() {
		Statistics statistics = entityManagerFactory().unwrap( SessionFactory.class ).getStatistics();
		statistics.clear();
		doInJPA(
				this::entityManagerFactory, entityManager -> {
					EntityGraph<AEntity> entityGraph = entityManager.createEntityGraph( AEntity.class );
					entityGraph.addAttributeNodes( "cList" );
					entityGraph.addSubgraph( "cList" ).addAttributeNodes( "a" );

					TypedQuery<AEntity> query = entityManager.createQuery(
							"select a from AEntity as a where a.id = :aid ",
							AEntity.class
					);
					query.setHint( GraphSemantic.LOAD.getJpaHintName(), entityGraph );
					query.setParameter( "aid", 1 );

					AEntity cEntity = query.getSingleResult();

					assertTrue( Hibernate.isInitialized( cEntity.getcList() ) );
					assertTrue( Hibernate.isInitialized( cEntity.getcList().get( 0 ) ) );

					assertEquals( 1L, statistics.getPrepareStatementCount() );
				} );
	}

if the test is correct, it passes with just

if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;
}

NathanQingyangXu · 2020-07-07T02:41:42Z

Hi @NathanQingyangXu still not sure we really need session.setFetchGraphLoadContext( null ); as discussed privately I tried to add the kind of test you think may need this call

@Test
	public void testQueryByIdWithLoadGraph3() {
		Statistics statistics = entityManagerFactory().unwrap( SessionFactory.class ).getStatistics();
		statistics.clear();
		doInJPA(
				this::entityManagerFactory, entityManager -> {
					EntityGraph<AEntity> entityGraph = entityManager.createEntityGraph( AEntity.class );
					entityGraph.addAttributeNodes( "cList" );
					entityGraph.addSubgraph( "cList" ).addAttributeNodes( "a" );

					TypedQuery<AEntity> query = entityManager.createQuery(
							"select a from AEntity as a where a.id = :aid ",
							AEntity.class
					);
					query.setHint( GraphSemantic.LOAD.getJpaHintName(), entityGraph );
					query.setParameter( "aid", 1 );

					AEntity cEntity = query.getSingleResult();

					assertTrue( Hibernate.isInitialized( cEntity.getcList() ) );
					assertTrue( Hibernate.isInitialized( cEntity.getcList().get( 0 ) ) );

					assertEquals( 1L, statistics.getPrepareStatementCount() );
				} );
	}

if the test is correct, it passes with just

if ( fetchGraphContext != null && !fetchGraphContext.appliesTo( entity.getClass() ) ) {
			LOG.warnf( "Entity graph specified is not applicable to the entity [%s]. Ignored.", entity);
			fetchGraphContext = null;
}

The reason your test didn't expose issue might be that cList will fail in type testing, ending up with context being set to null already. I only realized my original proposal won't work when I created my own.
I added a new testing case to showcase why session.setFetchGraphLoadContext( null ) is needed. It is hard to explain and even confusing, but it ensures that entity graph is disabled for scenario wherein it is not applicable.

dreab8 · 2020-07-07T07:19:03Z

Thanks @NathanQingyangXu for the additional test, may be I'm missing something but I tried it with just

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;

		}

and it passes.

NathanQingyangXu · 2020-07-07T12:13:10Z

Thanks @NathanQingyangXu for the additional test, may be I'm missing something but I tried it with just

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;

		}

and it passes.

Sorry for my premature pushing. Now I updated the testing case and I am confident it should be fine.

…ity graph

NathanQingyangXu · 2020-07-07T13:19:54Z

documentation/src/main/asciidoc/userguide/chapters/fetching/Fetching.adoc

+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:


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.

dreab8 · 2020-07-07T16:16:54Z

hibernate-core/src/main/java/org/hibernate/engine/internal/TwoPhaseLoad.java

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


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

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.

Sanne · 2020-07-09T13:09:06Z

thanks! Merging

NathanQingyangXu force-pushed the HHH-14097 branch from 271d0ff to 55d23b0 Compare July 4, 2020 14:48

Sanne reviewed Jul 6, 2020

View reviewed changes

documentation/src/test/java/org/hibernate/userguide/fetching/GraphFetchingTest.java Outdated Show resolved Hide resolved

dreab8 mentioned this pull request Jul 6, 2020

HHH-14097 Add test for issue #3452

Closed

NathanQingyangXu force-pushed the HHH-14097 branch 4 times, most recently from d0577d4 to 4660ca8 Compare July 6, 2020 11:46

Sanne reviewed Jul 6, 2020

View reviewed changes

NathanQingyangXu force-pushed the HHH-14097 branch from 4660ca8 to e7854c4 Compare July 7, 2020 02:38

NathanQingyangXu force-pushed the HHH-14097 branch from e7854c4 to fc85810 Compare July 7, 2020 12:11

HHH-14097 fix bug that redundant SQLs might be issued for 'FETCH' ent…

ee82443

…ity graph

NathanQingyangXu force-pushed the HHH-14097 branch from fc85810 to ee82443 Compare July 7, 2020 13:17

NathanQingyangXu commented Jul 7, 2020

View reviewed changes

dreab8 reviewed Jul 7, 2020

View reviewed changes

dreab8 approved these changes Jul 7, 2020

View reviewed changes

dreab8 requested a review from Sanne July 8, 2020 08:13

Sanne merged commit 5952c0a into hibernate:master Jul 9, 2020

Uh oh!

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

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

Uh oh!

Conversation

NathanQingyangXu commented Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sanne Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

dreab8 commented Jul 6, 2020

Uh oh!

NathanQingyangXu commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dreab8 commented Jul 7, 2020

Uh oh!

NathanQingyangXu commented Jul 7, 2020

Uh oh!

NathanQingyangXu Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

dreab8 Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanne commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NathanQingyangXu commented Jul 4, 2020 •

edited

Loading

NathanQingyangXu Jul 6, 2020 •

edited

Loading

NathanQingyangXu commented Jul 7, 2020 •

edited

Loading

NathanQingyangXu Jul 7, 2020 •

edited

Loading