HHH-17837 Render target-side key for explicit plural joins when needed

Also, change how we determine whether we need to use the target-side to only the strictly needed cases (non-optimizable joins, `group by` or `order by` clauses)

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/EntityAssociationMapping.java

@@ -6,6 +6,8 @@
  */
 package org.hibernate.metamodel.mapping;
 
+import java.util.Set;
+
 import org.hibernate.sql.ast.tree.from.TableGroupJoinProducer;
 
 /**
@@ -21,6 +23,8 @@ default String getFetchableName() {
 
 	EntityMappingType getAssociatedEntityMappingType();
 
+	Set<String> getTargetKeyPropertyNames();
+
 	/**
 	 * The model sub-part relative to the associated entity type that is the target
 	 * of this association's foreign-key

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/AbstractEntityCollectionPart.java

@@ -60,7 +60,7 @@ public abstract class AbstractEntityCollectionPart implements EntityCollectionPa
 	private final EntityMappingType associatedEntityTypeDescriptor;
 	private final NotFoundAction notFoundAction;
 
-	private final Set<String> targetKeyPropertyNames;
+	protected final Set<String> targetKeyPropertyNames;
 
 	public AbstractEntityCollectionPart(
 			Nature nature,
@@ -110,10 +110,6 @@ public EntityMappingType getMappedType() {
 		return getAssociatedEntityMappingType();
 	}
 
-	protected Set<String> getTargetKeyPropertyNames() {
-		return targetKeyPropertyNames;
-	}
-
 	@Override
 	public NavigableRole getNavigableRole() {
 		return navigableRole;

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java

@@ -7,6 +7,7 @@
 package org.hibernate.metamodel.mapping.internal;
 
 import java.util.Locale;
+import java.util.Set;
 import java.util.function.Consumer;
 
 import org.hibernate.annotations.NotFoundAction;
@@ -135,6 +136,11 @@ public ModelPart findSubPart(String name, EntityMappingType targetType) {
 		return super.findSubPart( name, targetType );
 	}
 
+	@Override
+	public Set<String> getTargetKeyPropertyNames() {
+		return targetKeyPropertyNames;
+	}
+
 	@Override
 	public <X, Y> int breakDownJdbcValues(
 			Object domainValue,

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java

@@ -899,6 +899,7 @@ public String getTargetKeyPropertyName() {
 		return targetKeyPropertyName;
 	}
 
+	@Override
 	public Set<String> getTargetKeyPropertyNames() {
 		return targetKeyPropertyNames;
 	}

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

@@ -25,13 +25,14 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.BasicValuedMapping;
 import org.hibernate.metamodel.mapping.Bindable;
+import org.hibernate.metamodel.mapping.CollectionPart;
 import org.hibernate.metamodel.mapping.EntityAssociationMapping;
 import org.hibernate.metamodel.mapping.EntityIdentifierMapping;
 import org.hibernate.metamodel.mapping.EntityMappingType;
 import org.hibernate.metamodel.mapping.ForeignKeyDescriptor;
 import org.hibernate.metamodel.mapping.JdbcMapping;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.MappingModelExpressible;
+import org.hibernate.metamodel.mapping.ModelPart;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
 import org.hibernate.query.IllegalQueryOperationException;
@@ -46,6 +47,7 @@
 import org.hibernate.query.sqm.SqmQuerySource;
 import org.hibernate.query.sqm.spi.JdbcParameterBySqmParameterAccess;
 import org.hibernate.query.sqm.spi.SqmParameterMappingModelResolutionAccess;
+import org.hibernate.query.sqm.sql.SqmToSqlAstConverter;
 import org.hibernate.query.sqm.tree.SqmDmlStatement;
 import org.hibernate.query.sqm.tree.SqmJoinType;
 import org.hibernate.query.sqm.tree.SqmStatement;
@@ -60,11 +62,13 @@
 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;
 import org.hibernate.query.sqm.tree.select.SqmSelectableNode;
 import org.hibernate.query.sqm.tree.select.SqmSelection;
 import org.hibernate.query.sqm.tree.select.SqmSortSpecification;
 import org.hibernate.spi.NavigablePath;
+import org.hibernate.sql.ast.Clause;
 import org.hibernate.sql.ast.SqlTreeCreationException;
 import org.hibernate.sql.ast.tree.expression.JdbcParameter;
 import org.hibernate.sql.ast.tree.from.TableGroup;
@@ -79,6 +83,7 @@
 import org.hibernate.type.internal.ConvertedBasicTypeImpl;
 import org.hibernate.type.spi.TypeConfiguration;
 
+import static org.hibernate.internal.util.NullnessUtil.castNonNull;
 import static org.hibernate.query.sqm.tree.jpa.ParameterCollector.collectParameters;
 
 /**
@@ -132,13 +137,56 @@ public static IllegalQueryOperationException expectingNonSelect(SqmStatement<?>
 	}
 
 	/**
-	 * 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.
+	 * Utility that returns the entity association target's mapping type if the specified {@code sqmPath} should
+	 * be dereferenced using the target table, i.e. when the path's lhs is an explicit join that is used in the
+	 * group by clause, or defaults to the provided {@code modelPartContainer} otherwise.
 	 */
-	public static boolean needsTargetTableMapping(SqmPath<?> sqmPath, ModelPartContainer modelPartContainer) {
-		return modelPartContainer.getPartMappingType() != modelPartContainer
-				&& sqmPath.getLhs() instanceof SqmFrom<?, ?>
-				&& modelPartContainer.getPartMappingType() instanceof ManagedMappingType;
+	public static ModelPartContainer getTargetMappingIfNeeded(
+			SqmPath<?> sqmPath,
+			ModelPartContainer modelPartContainer,
+			SqmToSqlAstConverter sqlAstCreationState) {
+		final SqmQueryPart<?> queryPart = sqlAstCreationState.getCurrentSqmQueryPart();
+		if ( queryPart != null ) {
+			// We only need to do this for queries
+			final Clause clause = sqlAstCreationState.getCurrentClauseStack().getCurrent();
+			if ( clause != Clause.FROM && modelPartContainer.getPartMappingType() != modelPartContainer && sqmPath.getLhs() instanceof SqmFrom<?, ?> ) {
+				final ModelPart modelPart;
+				if ( modelPartContainer instanceof PluralAttributeMapping ) {
+					modelPart = getCollectionPart(
+							(PluralAttributeMapping) modelPartContainer,
+							castNonNull( sqmPath.getNavigablePath().getParent() )
+					);
+				}
+				else {
+					modelPart = modelPartContainer;
+				}
+				if ( modelPart instanceof EntityAssociationMapping ) {
+					final EntityAssociationMapping association = (EntityAssociationMapping) modelPart;
+					// If the path is one of the association's target key properties,
+					// we need to render the target side if in group/order by
+					if ( association.getTargetKeyPropertyNames().contains( sqmPath.getReferencedPathSource().getPathName() )
+							&& ( clause == Clause.GROUP || clause == Clause.ORDER
+							|| !isFkOptimizationAllowed( sqmPath.getLhs() )
+							|| queryPart.getFirstQuerySpec().groupByClauseContains( sqmPath.getNavigablePath() ) ) ) {
+						return association.getAssociatedEntityMappingType();
+					}
+				}
+			}
+		}
+		return modelPartContainer;
+	}
+
+	private static CollectionPart getCollectionPart(PluralAttributeMapping attribute, NavigablePath path) {
+		final CollectionPart.Nature nature = CollectionPart.Nature.fromNameExact( path.getLocalName() );
+		if ( nature != null ) {
+			switch ( nature ) {
+				case ELEMENT:
+					return attribute.getElementDescriptor();
+				case INDEX:
+					return attribute.getIndexDescriptor();
+			}
+		}
+		return null;
 	}
 
 	/**

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

@@ -13,7 +13,6 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.BasicValuedModelPart;
 import org.hibernate.metamodel.mapping.EntityMappingType;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.MappingType;
 import org.hibernate.metamodel.mapping.ModelPart;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
@@ -35,7 +34,7 @@
 import org.hibernate.sql.ast.tree.update.Assignable;
 
 import static org.hibernate.internal.util.NullnessUtil.castNonNull;
-import static org.hibernate.query.sqm.internal.SqmUtil.needsTargetTableMapping;
+import static org.hibernate.query.sqm.internal.SqmUtil.getTargetMappingIfNeeded;
 
 /**
  * @author Steve Ebersole
@@ -82,20 +81,12 @@ public static <T> BasicValuedPathInterpretation<T> from(
 			}
 		}
 
-		final ModelPart modelPart;
-		if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) {
-			// We have to make sure we render the column of the target table
-			modelPart = ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
-		else {
-			modelPart = modelPartContainer.findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
+		// Use the target type to find the sub part if needed, otherwise just use the container
+		final ModelPart modelPart = getTargetMappingIfNeeded(
+				sqmPath,
+				modelPartContainer,
+				sqlAstCreationState
+		).findSubPart( sqmPath.getReferencedPathSource().getPathName(), treatTarget );
 
 		if ( modelPart == null ) {
 			if ( jpaQueryComplianceEnabled ) {

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

@@ -13,7 +13,6 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.EmbeddableValuedModelPart;
 import org.hibernate.metamodel.mapping.EntityMappingType;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
 import org.hibernate.metamodel.model.domain.EntityDomainType;
 import org.hibernate.query.sqm.sql.SqmToSqlAstConverter;
@@ -29,7 +28,7 @@
 import org.hibernate.sql.ast.tree.from.TableGroup;
 import org.hibernate.sql.ast.tree.update.Assignable;
 
-import static org.hibernate.query.sqm.internal.SqmUtil.needsTargetTableMapping;
+import static org.hibernate.query.sqm.internal.SqmUtil.getTargetMappingIfNeeded;
 
 /**
  * @author Steve Ebersole
@@ -65,20 +64,12 @@ else if ( lhs.getNodeType() instanceof EntityDomainType ) {
 		}
 
 		final ModelPartContainer modelPartContainer = tableGroup.getModelPart();
-		final EmbeddableValuedModelPart mapping;
-		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
-			);
-		}
-		else {
-			mapping = (EmbeddableValuedModelPart) modelPartContainer.findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
+		// Use the target type to find the sub part if needed, otherwise just use the container
+		final EmbeddableValuedModelPart mapping = (EmbeddableValuedModelPart) getTargetMappingIfNeeded(
+				sqmPath,
+				modelPartContainer,
+				sqlAstCreationState
+		).findSubPart( sqmPath.getReferencedPathSource().getPathName(), treatTarget );
 
 		return new EmbeddableValuedPathInterpretation<>(
 				mapping.toSqlExpression(

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

@@ -162,7 +162,9 @@ private static <T> EntityValuedPathInterpretation<T> from(
 		// we try to make use of it and the FK model part if possible based on the inferred mapping
 		if ( mapping instanceof EntityAssociationMapping ) {
 			final EntityAssociationMapping associationMapping = (EntityAssociationMapping) mapping;
-			final ModelPart keyTargetMatchPart = associationMapping.getKeyTargetMatchPart();
+			final ModelPart keyTargetMatchPart = associationMapping.getForeignKeyDescriptor().getPart(
+					associationMapping.getSideNature()
+			);
 
 			if ( associationMapping.isFkOptimizationAllowed() ) {
 				final boolean forceUsingForeignKeyAssociationSidePart;

hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java

@@ -628,8 +628,8 @@ public void appendHqlString(StringBuilder sb) {
 	}
 
 	public boolean groupByClauseContains(NavigablePath path) {
-		for ( SqmExpression<?> expression : groupByClauseExpressions ) {
-			if ( expression instanceof SqmPath && ( (SqmPath<?>) expression ).getNavigablePath() == path ) {
+		for ( final SqmExpression<?> expression : groupByClauseExpressions ) {
+			if ( expression instanceof SqmPath && ( (SqmPath<?>) expression ).getNavigablePath().isParentOrEqual( path ) ) {
 				return true;
 			}
 		}

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

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

@@ -152,8 +152,9 @@ public void testHqlSelectSon(SessionFactoryScope scope) {
 							.getSingleResult();
 
 					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 );
+					// 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 );
 					statementInspector.assertNumberOfOccurrenceInQuery( 1, "join", 3 );
 					assertThat( son.getParent(), CoreMatchers.notNullValue() );