HHH-15342 Inappropriate variation of HQL left join to SQL inner join

Author: beikov

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

@@ -559,7 +559,12 @@ public TableGroupJoin createTableGroupJoin(
 			SqlAstCreationContext creationContext) {
 		final SqlAstJoinType joinType;
 		if ( requestedJoinType == null ) {
-			joinType = SqlAstJoinType.INNER;
+			if ( fetched ) {
+				joinType = getDefaultSqlAstJoinType( lhs );
+			}
+			else {
+				joinType = SqlAstJoinType.INNER;
+			}
 		}
 		else {
 			joinType = requestedJoinType;

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

@@ -100,8 +100,6 @@
 import org.hibernate.type.EntityType;
 import org.hibernate.type.Type;
 
-import static java.util.Objects.requireNonNullElse;
-
 /**
  * @author Steve Ebersole
  */
@@ -1221,9 +1219,8 @@ public static class EntityB {
 
 		 having the left join we don't want to add an extra implicit join that will be translated into an SQL inner join (see HHH-15342)
 		*/
-		if ( ( ( hasNotFoundAction() && doesNotExistATableGroupJoin( parentTableGroup, fetchablePath ) )
-				|| ( fetchTiming == FetchTiming.IMMEDIATE && selected ) ) ) {
-			final TableGroup tableGroup = determineTableGroup(
+		if ( fetchTiming == FetchTiming.IMMEDIATE && selected || hasNotFoundAction() ) {
+			final TableGroup tableGroup = determineTableGroupForFetch(
 					fetchablePath,
 					fetchParent,
 					parentTableGroup,
@@ -1332,58 +1329,66 @@ public static class EntityB {
 		);
 	}
 
-	private boolean doesNotExistATableGroupJoin(TableGroup parentTableGroup, NavigablePath fetchablePath) {
-		for ( TableGroupJoin tableGroupJoin : parentTableGroup.getTableGroupJoins() ) {
-			if ( tableGroupJoin.getNavigablePath().pathsMatch( fetchablePath ) ) {
-				return false;
-			}
-		}
-		return true;
-	}
-
-	private TableGroup determineTableGroup(NavigablePath fetchablePath, FetchParent fetchParent, TableGroup parentTableGroup, String resultVariable, FromClauseAccess fromClauseAccess, DomainResultCreationState creationState) {
-		final TableGroup tableGroup;
+	private TableGroup determineTableGroupForFetch(
+			NavigablePath fetchablePath,
+			FetchParent fetchParent,
+			TableGroup parentTableGroup,
+			String resultVariable,
+			FromClauseAccess fromClauseAccess,
+			DomainResultCreationState creationState) {
+		final SqlAstJoinType joinType;
 		if ( fetchParent instanceof EntityResultJoinedSubclassImpl
 				&& ( (EntityPersister) fetchParent.getReferencedModePart() ).findDeclaredAttributeMapping( getPartName() ) == null ) {
-			final TableGroupJoin tableGroupJoin = createTableGroupJoin(
-					fetchablePath,
-					parentTableGroup,
-					resultVariable,
-					getJoinType( fetchablePath, parentTableGroup ),
-					true,
-					false,
-					creationState.getSqlAstCreationState()
-			);
-			if ( hasNotFoundAction() ) {
-				tableGroupJoin.setImplicit();
-			}
-
-			parentTableGroup.addTableGroupJoin( tableGroupJoin );
-			tableGroup = tableGroupJoin.getJoinedGroup();
-			fromClauseAccess.registerTableGroup( fetchablePath, tableGroup );
+			joinType = getJoinTypeForFetch( fetchablePath, parentTableGroup );
 		}
 		else {
-			tableGroup = fromClauseAccess.resolveTableGroup(
-					fetchablePath,
-					np -> {
-						final TableGroupJoin tableGroupJoin = createTableGroupJoin(
-								fetchablePath,
-								parentTableGroup,
-								resultVariable,
-								getDefaultSqlAstJoinType( parentTableGroup ),
-								true,
-								false,
-								creationState.getSqlAstCreationState()
-						);
-						if ( hasNotFoundAction() ) {
-							tableGroupJoin.setImplicit();
+			joinType = null;
+		}
+		return fromClauseAccess.resolveTableGroup(
+				fetchablePath,
+				np -> {
+					// Try to reuse an existing join if possible,
+					// and note that we prefer reusing an inner over a left join,
+					// because a left join might stay uninitialized if unused
+					TableGroup leftJoined = null;
+					for ( TableGroupJoin tableGroupJoin : parentTableGroup.getTableGroupJoins() ) {
+						switch ( tableGroupJoin.getJoinType() ) {
+							case INNER:
+								// If this is an inner joins, it's fine if the paths match
+								// Since this inner join would filter the parent row anyway,
+								// it makes no sense to add another left join for this association
+								if ( tableGroupJoin.getNavigablePath().pathsMatch( np ) ) {
+									return tableGroupJoin.getJoinedGroup();
+								}
+								break;
+							case LEFT:
+								// For an existing left join on the other hand which is row preserving,
+								// it is important to check if the predicate has user defined bits in it
+								// and only if it doesn't, we can reuse the join
+								if ( tableGroupJoin.getNavigablePath().pathsMatch( np )
+										&& isSimpleJoinPredicate( tableGroupJoin.getPredicate() ) ) {
+									leftJoined = tableGroupJoin.getJoinedGroup();
+								}
 						}
-						parentTableGroup.addTableGroupJoin( tableGroupJoin );
-						return tableGroupJoin.getJoinedGroup();
 					}
-			);
-		}
-		return tableGroup;
+
+					if ( leftJoined != null ) {
+						return leftJoined;
+					}
+
+					final TableGroupJoin tableGroupJoin = createTableGroupJoin(
+							fetchablePath,
+							parentTableGroup,
+							resultVariable,
+							joinType,
+							true,
+							false,
+							creationState.getSqlAstCreationState()
+					);
+					parentTableGroup.addTableGroupJoin( tableGroupJoin );
+					return tableGroupJoin.getJoinedGroup();
+				}
+		);
 	}
 
 	private boolean isSelectByUniqueKey(ForeignKeyDescriptor.Nature side) {
@@ -1531,7 +1536,9 @@ else if ( parentTableGroup.getModelPart() instanceof CollectionPart ) {
 
 	@Override
 	public boolean isSimpleJoinPredicate(Predicate predicate) {
-		return foreignKeyDescriptor.isSimpleJoinPredicate( predicate );
+		// Since the table group is lazy, the initial predicate is null,
+		// but if we get null here, we can safely assume this will be a simple join predicate
+		return predicate == null || foreignKeyDescriptor.isSimpleJoinPredicate( predicate );
 	}
 
 	@Override
@@ -1555,7 +1562,18 @@ public TableGroupJoin createTableGroupJoin(
 		// This is vital for the map key property check that comes next
 		assert !( lhs instanceof PluralTableGroup );
 
-		final SqlAstJoinType joinType = requireNonNullElse( requestedJoinType, SqlAstJoinType.INNER );
+		final SqlAstJoinType joinType;
+		if ( requestedJoinType == null ) {
+			if ( fetched ) {
+				joinType = getDefaultSqlAstJoinType( lhs );
+			}
+			else {
+				joinType = SqlAstJoinType.INNER;
+			}
+		}
+		else {
+			joinType = requestedJoinType;
+		}
 
 		// If a parent is a collection part, there is no custom predicate and the join is INNER or LEFT
 		// we check if this attribute is the map key property to reuse the existing index table group
@@ -1800,13 +1818,13 @@ private void initializeIfNeeded(TableGroup lhs, SqlAstJoinType sqlAstJoinType, T
 		}
 	}
 
-	private SqlAstJoinType getJoinType(NavigablePath navigablePath, TableGroup tableGroup) {
+	private SqlAstJoinType getJoinTypeForFetch(NavigablePath navigablePath, TableGroup tableGroup) {
 		for ( TableGroupJoin tableGroupJoin : tableGroup.getTableGroupJoins() ) {
 			if ( tableGroupJoin.getNavigablePath().equals( navigablePath ) ) {
 				return tableGroupJoin.getJoinType();
 			}
 		}
-		return getDefaultSqlAstJoinType( tableGroup );
+		return null;
 	}
 
 	public TableGroup createTableGroupInternal(

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

@@ -3312,7 +3312,6 @@ private TableGroup createTableGroup(TableGroup parentTableGroup, SqmPath<?> join
 							false,
 							this
 					);
-					tableGroupJoin.setImplicit();
 					// Implicit joins in the ON clause of attribute joins need to be added as nested table group joins
 					// We don't have to do that for entity joins etc. as these do not have an inherent dependency on the lhs.
 					// We can just add the implicit join before the currently processing join
@@ -6987,7 +6986,7 @@ else if ( getLoadQueryInfluencers().hasEnabledFetchProfiles() ) {
 									fetchablePath,
 									lhs,
 									alias,
-									tableGroupJoinProducer.getDefaultSqlAstJoinType( lhs ),
+									null,
 									true,
 									false,
 									BaseSqmToSqlAstConverter.this

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

@@ -7,6 +7,7 @@
 package org.hibernate.query.sqm.sql.internal;
 
 import org.hibernate.NotYetImplementedFor6Exception;
+import org.hibernate.metamodel.mapping.CollectionPart;
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
 import org.hibernate.spi.NavigablePath;
 import org.hibernate.query.sqm.sql.SqmToSqlAstConverter;
@@ -60,7 +61,7 @@ public DomainResult<T> createDomainResult(String resultVariable, DomainResultCre
 		// This is only invoked when a plural attribute is a top level select, order by or group by item
 		// in which case we have to produce results for the element
 		return ( (PluralAttributeMapping) getExpressionType() ).getElementDescriptor().createDomainResult(
-				getNavigablePath(),
+				getNavigablePath().append( CollectionPart.Nature.ELEMENT.getName() ),
 				getTableGroup(),
 				resultVariable,
 				creationState

hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroupJoin.java

@@ -27,8 +27,6 @@ public class TableGroupJoin implements TableJoin, DomainResultProducer {
 	private SqlAstJoinType joinType;
 	private Predicate predicate;
 
-	private boolean implicit;
-
 	public TableGroupJoin(
 			NavigablePath navigablePath,
 			SqlAstJoinType joinType,
@@ -93,12 +91,8 @@ public NavigablePath getNavigablePath() {
 		return navigablePath;
 	}
 
-	public void setImplicit(){
-		this.implicit = true;
-	}
-
-	public boolean isImplicit(){
-		return implicit;
+	public boolean isImplicit() {
+		return !navigablePath.isAliased();
 	}
 
 	@Override