HHH-17706 Optimize FK comparison to eliminate unnecessary left join

Fix regression introduced by ef155c2

Author: quaff

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<T> extends AbstractSqmPathInterpretation<T> implements Assignable,  DomainResultProducer<T> {
 	/**
@@ -83,7 +88,7 @@ public static <T> BasicValuedPathInterpretation<T> 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(

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

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<OtherEntity> cq = cb.createQuery( OtherEntity.class );
+					Root<OtherEntity> 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(

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