From 0942883063f0b9ab2a15c7affab696904700d4fa Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Sun, 4 Feb 2024 12:58:14 +0800 Subject: [PATCH] HHH-17706 Optimize FK comparison to eliminate unnecessary left join Fix regression introduced by ef155c22c1c2e57d0ce9fcc21f506efa1bba5be2 --- .../BasicValuedPathInterpretation.java | 30 ++++++++++++++++++- .../orm/test/jpa/ql/MapIssueTest.java | 4 +-- .../sql/exec/manytoone/ManyToOneTest.java | 26 ++++++++++++++-- ...ssociationsOneOfWhichIsAJoinTableTest.java | 2 +- 4 files changed, 55 insertions(+), 7 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java index b3691da05bab..a703755fd5bd 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java @@ -25,6 +25,9 @@ import org.hibernate.query.sqm.tree.domain.SqmBasicValuedSimplePath; import org.hibernate.query.sqm.tree.domain.SqmPath; import org.hibernate.query.sqm.tree.domain.SqmTreatedPath; +import org.hibernate.query.sqm.tree.select.SqmOrderByClause; +import org.hibernate.query.sqm.tree.select.SqmQuerySpec; +import org.hibernate.query.sqm.tree.select.SqmSelectClause; import org.hibernate.spi.NavigablePath; import org.hibernate.sql.ast.SqlAstWalker; import org.hibernate.sql.ast.tree.expression.ColumnReference; @@ -35,10 +38,12 @@ import org.hibernate.sql.ast.tree.update.Assignable; import static org.hibernate.internal.util.NullnessUtil.castNonNull; +import static org.hibernate.query.sqm.internal.SqmUtil.isFkOptimizationAllowed; import static org.hibernate.query.sqm.internal.SqmUtil.needsTargetTableMapping; /** * @author Steve Ebersole + * @author Yanming Zhou */ public class BasicValuedPathInterpretation extends AbstractSqmPathInterpretation implements Assignable, DomainResultProducer { /** @@ -83,7 +88,7 @@ public static BasicValuedPathInterpretation from( } final ModelPart modelPart; - if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) { + if ( !isFkOptimizationAllowedForState( sqmPath.getLhs(), sqlAstCreationState ) && needsTargetTableMapping( sqmPath, modelPartContainer ) ) { // We have to make sure we render the column of the target table modelPart = ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart( sqmPath.getReferencedPathSource().getPathName(), @@ -140,6 +145,29 @@ else if ( expression instanceof SqlSelectionExpression ) { return new BasicValuedPathInterpretation<>( columnReference, sqmPath.getNavigablePath(), mapping, tableGroup ); } + private static boolean isFkOptimizationAllowedForState(SqmPath sqmPath, SqmToSqlAstConverter sqlAstCreationState) { + boolean isFkOptimizationAllowed = isFkOptimizationAllowed( sqmPath ); + if ( isFkOptimizationAllowed ) { + if ( sqlAstCreationState.getCurrentSqmQueryPart() instanceof SqmQuerySpec ) { + final SqmQuerySpec spec = (SqmQuerySpec) sqlAstCreationState.getCurrentSqmQueryPart(); + final SqmOrderByClause orderByClause = spec.getOrderByClause(); + if ( orderByClause != null && !orderByClause.getSortSpecifications().isEmpty() ) { + final SqmSelectClause selectClause = spec.getSelectClause(); + if ( selectClause != null && selectClause.isDistinct() ) { + // DISTINCT query requires sorted column in SELECT list + isFkOptimizationAllowed = false; + } + if ( !spec.getGroupByClauseExpressions().isEmpty() ) { + // PostgreSQL requires sorted column appear in the GROUP BY clause or be used in an aggregate function + isFkOptimizationAllowed = false; + } + } + + } + } + return isFkOptimizationAllowed; + } + private final ColumnReference columnReference; public BasicValuedPathInterpretation( diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java index 5f58d9dcb506..e17725c0dc94 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java @@ -59,8 +59,8 @@ public void testMapKeyJoinIsIncluded(SessionFactoryScope scope) { s -> { s.createQuery( "select c from MapOwner as o join o.contents c join c.relationship r where r.id is not null" ).list(); statementInspector.assertExecutedCount( 1 ); - // Assert 3 joins, collection table, collection element and relationship - statementInspector.assertNumberOfJoins( 0, 3 ); + // Assert 2 joins, collection table, collection element + statementInspector.assertNumberOfJoins( 0, 2 ); } ); } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/manytoone/ManyToOneTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/manytoone/ManyToOneTest.java index 2bd5c7c72d9e..e9349633280b 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/manytoone/ManyToOneTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/manytoone/ManyToOneTest.java @@ -9,14 +9,16 @@ import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.Id; -import jakarta.persistence.JoinColumn; -import jakarta.persistence.JoinTable; import jakarta.persistence.ManyToOne; import jakarta.persistence.Table; +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.JoinType; +import jakarta.persistence.criteria.Root; import org.hibernate.Hibernate; import org.hibernate.stat.spi.StatisticsImplementor; - +import org.hibernate.testing.jdbc.SQLStatementInspector; import org.hibernate.testing.orm.junit.DomainModel; import org.hibernate.testing.orm.junit.ServiceRegistry; import org.hibernate.testing.orm.junit.SessionFactory; @@ -34,6 +36,7 @@ /** * @author Andrea Boriero + * @author Yanming Zhou */ @DomainModel( annotatedClasses = { @@ -304,6 +307,23 @@ public void testDelete(SessionFactoryScope scope) { ); } + @Test + public void testFkOptimizationWithLeftJoin(SessionFactoryScope scope) { + SQLStatementInspector statementInspector = scope.getCollectingStatementInspector(); + statementInspector.clear(); + scope.inTransaction( + session -> { + CriteriaBuilder cb = session.getCriteriaBuilder(); + CriteriaQuery cq = cb.createQuery( OtherEntity.class ); + Root root = cq.from( OtherEntity.class ); + cq.select(root).where( cb.equal( root.join("simpleEntity", JoinType.LEFT ).get( "id" ), 1 ) ); + assertThat( session.createQuery( cq ).getResultList().size(), is(1) ); + statementInspector.assertExecutedCount( 1 ); + statementInspector.assertNumberOfOccurrenceInQuery( 0, "join", 0 ); + } + ); + } + @BeforeEach public void setUp(SessionFactoryScope scope) { scope.inTransaction( diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java index 0a780c481392..80b7f11aaaa0 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java @@ -153,7 +153,7 @@ public void testHqlSelectSon(SessionFactoryScope scope) { statementInspector.assertExecutedCount( 2 ); // The join to the target table PARENT for Male#parent is added since it's explicitly joined in HQL - statementInspector.assertNumberOfOccurrenceInQuery( 0, "join", 2 ); + statementInspector.assertNumberOfOccurrenceInQuery( 0, "join", 1 ); statementInspector.assertNumberOfOccurrenceInQuery( 1, "join", 3 ); assertThat( son.getParent(), CoreMatchers.notNullValue() );