HHH-17379 HHH-17397 Improve check for non-optimizable path expressions

Author: mbladel

hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java

@@ -55,6 +55,7 @@
 import org.hibernate.query.sqm.tree.expression.SqmParameter;
 import org.hibernate.query.sqm.tree.from.SqmFrom;
 import org.hibernate.query.sqm.tree.from.SqmJoin;
+import org.hibernate.query.sqm.tree.from.SqmQualifiedJoin;
 import org.hibernate.query.sqm.tree.from.SqmRoot;
 import org.hibernate.query.sqm.tree.select.SqmQueryPart;
 import org.hibernate.query.sqm.tree.select.SqmSelectStatement;
@@ -127,27 +128,33 @@ public static IllegalQueryOperationException expectingNonSelect(SqmStatement<?>
 		);
 	}
 
-	public static boolean needsTargetTableMapping(
-			SqmPath<?> sqmPath,
-			ModelPartContainer modelPartContainer,
-			SqmToSqlAstConverter sqlAstCreationState) {
-		final Clause currentClause = sqlAstCreationState.getCurrentClauseStack().getCurrent();
-		return ( currentClause == Clause.GROUP || currentClause == Clause.SELECT || currentClause == Clause.ORDER || currentClause == Clause.HAVING )
-				&& modelPartContainer.getPartMappingType() != modelPartContainer
+	/**
+	 * Utility that returns {@code true} if the specified {@link SqmPath sqmPath} should be
+	 * dereferenced using the target table mapping, i.e. when the path's lhs is an explicit join.
+	 */
+	public static boolean needsTargetTableMapping(SqmPath<?> sqmPath, ModelPartContainer modelPartContainer) {
+		return modelPartContainer.getPartMappingType() != modelPartContainer
 				&& sqmPath.getLhs() instanceof SqmFrom<?, ?>
-				&& modelPartContainer.getPartMappingType() instanceof ManagedMappingType
-				&& ( groupByClauseContains( sqlAstCreationState.getCurrentSqmQueryPart(), sqmPath.getNavigablePath() )
-				|| isNonOptimizableJoin( sqmPath.getLhs() ) );
-	}
-
-	private static boolean groupByClauseContains(SqmQueryPart<?> sqmQueryPart, NavigablePath path) {
-		return sqmQueryPart.isSimpleQueryPart() && sqmQueryPart.getFirstQuerySpec().groupByClauseContains( path );
+				&& modelPartContainer.getPartMappingType() instanceof ManagedMappingType;
 	}
 
-	private static boolean isNonOptimizableJoin(SqmPath<?> sqmPath) {
+	/**
+	 * Utility that returns {@code false} when the provided {@link SqmPath sqmPath} is
+	 * a join that cannot be dereferenced through the foreign key on the associated table,
+	 * i.e. a join that's neither {@linkplain SqmJoinType#INNER} nor {@linkplain SqmJoinType#LEFT}
+	 * or one that has an explicit on clause predicate.
+	 */
+	public static boolean isFkOptimizationAllowed(SqmPath<?> sqmPath) {
 		if ( sqmPath instanceof SqmJoin<?, ?> ) {
-			final SqmJoinType sqmJoinType = ( (SqmJoin<?, ?>) sqmPath ).getSqmJoinType();
-			return sqmJoinType != SqmJoinType.INNER && sqmJoinType != SqmJoinType.LEFT;
+			final SqmJoin<?, ?> sqmJoin = (SqmJoin<?, ?>) sqmPath;
+			switch ( sqmJoin.getSqmJoinType() ) {
+				case INNER:
+				case LEFT:
+					return !( sqmJoin instanceof SqmQualifiedJoin<?, ?>)
+							|| ( (SqmQualifiedJoin<?, ?>) sqmJoin ).getJoinPredicate() == null;
+				default:
+					return false;
+			}
 		}
 		return false;
 	}

hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java

@@ -223,7 +223,6 @@
 import org.hibernate.query.sqm.tree.from.SqmFrom;
 import org.hibernate.query.sqm.tree.from.SqmFromClause;
 import org.hibernate.query.sqm.tree.from.SqmJoin;
-import org.hibernate.query.sqm.tree.from.SqmQualifiedJoin;
 import org.hibernate.query.sqm.tree.from.SqmRoot;
 import org.hibernate.query.sqm.tree.insert.SqmInsertSelectStatement;
 import org.hibernate.query.sqm.tree.insert.SqmInsertStatement;
@@ -431,6 +430,7 @@
 import static org.hibernate.query.sqm.TemporalUnit.NATIVE;
 import static org.hibernate.query.sqm.TemporalUnit.SECOND;
 import static org.hibernate.query.sqm.UnaryArithmeticOperator.UNARY_MINUS;
+import static org.hibernate.query.sqm.internal.SqmUtil.isFkOptimizationAllowed;
 import static org.hibernate.sql.ast.spi.SqlAstTreeHelper.combinePredicates;
 import static org.hibernate.type.spi.TypeConfiguration.isDuration;
 
@@ -3996,10 +3996,6 @@ public Expression visitQualifiedEntityJoin(SqmEntityJoin<?> sqmJoin) {
 		throw new InterpretationException( "SqmEntityJoin not yet resolved to TableGroup" );
 	}
 
-	private boolean isJoinWithPredicate(SqmFrom<?, ?> path) {
-		return path instanceof SqmQualifiedJoin && ( (SqmQualifiedJoin<?, ?>) path ).getJoinPredicate() != null;
-	}
-
 	private Expression visitTableGroup(TableGroup tableGroup, SqmFrom<?, ?> path) {
 		final ModelPartContainer tableGroupModelPart = tableGroup.getModelPart();
 
@@ -4026,9 +4022,9 @@ private Expression visitTableGroup(TableGroup tableGroup, SqmFrom<?, ?> path) {
 				// expansion to all target columns for select and group by clauses is handled in EntityValuedPathInterpretation
 				if ( entityValuedModelPart instanceof EntityAssociationMapping
 						&& ( (EntityAssociationMapping) entityValuedModelPart ).isFkOptimizationAllowed()
-						&& !isJoinWithPredicate( path ) ) {
+						&& isFkOptimizationAllowed( path ) ) {
 					// If the table group uses an association mapping that is not a one-to-many,
-					// we make use of the FK model part - unless the path is a join with an explicit predicate,
+					// we make use of the FK model part - unless the path is a non-optimizable join,
 					// for which we should always use the target's identifier to preserve semantics
 					final EntityAssociationMapping associationMapping = (EntityAssociationMapping) entityValuedModelPart;
 					final ModelPart targetPart = associationMapping.getForeignKeyDescriptor().getPart(

hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java

@@ -82,10 +82,8 @@ public static <T> BasicValuedPathInterpretation<T> from(
 		}
 
 		final BasicValuedModelPart mapping;
-		if ( needsTargetTableMapping( sqmPath, modelPartContainer, sqlAstCreationState ) ) {
-			// In the select, group by, order by and having clause we have to make sure we render
-			// the column of the target table, never the FK column, if the lhs is a join type that
-			// requires it (right, full) or if this path is contained in group by clause
+		if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) {
+			// We have to make sure we render the column of the target table
 			mapping = (BasicValuedModelPart) ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
 					sqmPath.getReferencedPathSource().getPathName(),
 					treatTarget

hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java

@@ -66,10 +66,8 @@ else if ( lhs.getNodeType() instanceof EntityDomainType ) {
 
 		final ModelPartContainer modelPartContainer = tableGroup.getModelPart();
 		final EmbeddableValuedModelPart mapping;
-		if ( needsTargetTableMapping( sqmPath, modelPartContainer, sqlAstCreationState ) ) {
-			// In the select, group by, order by and having clause we have to make sure we render
-			// the column of the target table, never the FK column, if the lhs is a join type that
-			// requires it (right, full) or if this path is contained in group by clause
+		if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) {
+			// We have to make sure we render the column of the target table
 			mapping = (EmbeddableValuedModelPart) ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
 					sqmPath.getReferencedPathSource().getPathName(),
 					treatTarget

hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java

@@ -52,15 +52,15 @@ public void testOnlyCollectionTableJoined(SessionFactoryScope scope) {
 	}
 
 	@Test
-	public void testMapKeyJoinIsOmitted(SessionFactoryScope scope) {
+	public void testMapKeyJoinIsIncluded(SessionFactoryScope scope) {
 		SQLStatementInspector statementInspector = scope.getCollectingStatementInspector();
 		statementInspector.clear();
 		scope.inTransaction(
 				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 2 joins, collection table and collection element. No need to join the relationship because it is not nullable
-					statementInspector.assertNumberOfJoins( 0, 2 );
+					// Assert 3 joins, collection table, collection element and relationship
+					statementInspector.assertNumberOfJoins( 0, 3 );
 				}
 		);
 	}

hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java

@@ -152,9 +152,8 @@ public void testHqlSelectSon(SessionFactoryScope scope) {
 							.getSingleResult();
 
 					statementInspector.assertExecutedCount( 2 );
-					// The join to the target table PARENT for Male#parent is avoided,
-					// because the FK in the collection table is not-null and data from the target table is not needed
-					statementInspector.assertNumberOfOccurrenceInQuery( 0, "join", 1 );
+					// 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( 1, "join", 3 );
 					assertThat( son.getParent(), CoreMatchers.notNullValue() );