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,62 @@ public static IllegalQueryOperationException expectingNonSelect(SqmStatement<?>
 		);
 	}
 
+	/**
+	 * Utility that returns {@code true} if the specified {@link SqmPath sqmPath} has to be
+	 * dereferenced using the target table mapping, and does not support fk optimization.
+	 * <p>
+	 * This is the case when the path is used in both the {@linkplain Clause#GROUP group by} clause
+	 * and a clause which {@linkplain #isClauseDependantOnGroupBy depends on it} or when the left
+	 * hand side of the path is a {@linkplain #isNonOptimizableJoin non-optimizable join}.
+	 */
 	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
+		return modelPartContainer.getPartMappingType() != modelPartContainer
 				&& sqmPath.getLhs() instanceof SqmFrom<?, ?>
 				&& modelPartContainer.getPartMappingType() instanceof ManagedMappingType
-				&& ( groupByClauseContains( sqlAstCreationState.getCurrentSqmQueryPart(), sqmPath.getNavigablePath() )
+				&& ( groupByClauseContains( sqlAstCreationState, sqmPath.getNavigablePath() )
 				|| isNonOptimizableJoin( sqmPath.getLhs() ) );
 	}
 
-	private static boolean groupByClauseContains(SqmQueryPart<?> sqmQueryPart, NavigablePath path) {
-		return sqmQueryPart.isSimpleQueryPart() && sqmQueryPart.getFirstQuerySpec().groupByClauseContains( path );
+	private static boolean groupByClauseContains(SqmToSqlAstConverter sqlAstCreationState, NavigablePath path) {
+		if ( isClauseDependantOnGroupBy( sqlAstCreationState.getCurrentClauseStack().getCurrent() ) ) {
+			final SqmQueryPart<?> sqmQueryPart = sqlAstCreationState.getCurrentSqmQueryPart();
+			return sqmQueryPart.isSimpleQueryPart() && sqmQueryPart.getFirstQuerySpec().groupByClauseContains( path );
+		}
+		return false;
+	}
+
+	private static boolean isClauseDependantOnGroupBy(Clause clause) {
+		switch ( clause ) {
+			case SELECT:
+			case GROUP:
+			case ORDER:
+			case HAVING:
+				return true;
+			default:
+				return false;
+		}
 	}
 
-	private static boolean isNonOptimizableJoin(SqmPath<?> sqmPath) {
+	/**
+	 * Utility that returns {@code true} 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 isNonOptimizableJoin(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 true;
+			}
 		}
 		return false;
 	}

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

@@ -431,6 +431,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.isNonOptimizableJoin;
 import static org.hibernate.sql.ast.spi.SqlAstTreeHelper.combinePredicates;
 import static org.hibernate.type.spi.TypeConfiguration.isDuration;
 
@@ -3998,10 +3999,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();
 
@@ -4028,9 +4025,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 ) ) {
+						&& !isNonOptimizableJoin( 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

@@ -83,9 +83,7 @@ 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
+			// 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

@@ -67,9 +67,7 @@ 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
+			// We have to make sure we render the column of the target table
 			mapping = (EmbeddableValuedModelPart) ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
 					sqmPath.getReferencedPathSource().getPathName(),
 					treatTarget