From 9ba0dd7af03d63dc39ce0ac8acd6688803ea0af9 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Fri, 22 Mar 2024 11:15:10 +0100 Subject: [PATCH] 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) --- .../mapping/EntityAssociationMapping.java | 4 ++ .../AbstractEntityCollectionPart.java | 6 +- .../internal/ManyToManyCollectionPart.java | 6 ++ .../internal/ToOneAttributeMapping.java | 1 + .../hibernate/query/sqm/internal/SqmUtil.java | 62 ++++++++++++++++--- .../BasicValuedPathInterpretation.java | 23 +++---- .../EmbeddableValuedPathInterpretation.java | 23 +++---- .../EntityValuedPathInterpretation.java | 4 +- .../query/sqm/tree/select/SqmQuerySpec.java | 4 +- .../orm/test/jpa/ql/MapIssueTest.java | 6 +- ...ssociationsOneOfWhichIsAJoinTableTest.java | 5 +- 11 files changed, 92 insertions(+), 52 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/EntityAssociationMapping.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/EntityAssociationMapping.java index e84b8143c0..a9ef7203ac 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/EntityAssociationMapping.java +++ b/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 @@ public interface EntityAssociationMapping extends ModelPart, Association, TableG EntityMappingType getAssociatedEntityMappingType(); + Set getTargetKeyPropertyNames(); + /** * The model sub-part relative to the associated entity type that is the target * of this association's foreign-key diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/AbstractEntityCollectionPart.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/AbstractEntityCollectionPart.java index 2856df0a6a..37728b47a7 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/AbstractEntityCollectionPart.java +++ b/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 targetKeyPropertyNames; + protected final Set targetKeyPropertyNames; public AbstractEntityCollectionPart( Nature nature, @@ -110,10 +110,6 @@ public abstract class AbstractEntityCollectionPart implements EntityCollectionPa return getAssociatedEntityMappingType(); } - protected Set getTargetKeyPropertyNames() { - return targetKeyPropertyNames; - } - @Override public NavigableRole getNavigableRole() { return navigableRole; diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java index fe52a9d710..4e262fc3fc 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java +++ b/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 class ManyToManyCollectionPart extends AbstractEntityCollectionPart imple return super.findSubPart( name, targetType ); } + @Override + public Set getTargetKeyPropertyNames() { + return targetKeyPropertyNames; + } + @Override public int breakDownJdbcValues( Object domainValue, diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java index b516f39d9c..ab1f542ead 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java +++ b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ToOneAttributeMapping.java @@ -899,6 +899,7 @@ public class ToOneAttributeMapping return targetKeyPropertyName; } + @Override public Set getTargetKeyPropertyNames() { return targetKeyPropertyNames; } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java index fb9bca60d7..34d8b76564 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java @@ -25,13 +25,14 @@ import org.hibernate.internal.util.collections.CollectionHelper; 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.SqmPathSource; 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.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; 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.BasicTypeImpl; 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 class SqmUtil { } /** - * 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; } /** diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java index b3691da05b..7253c3743c 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java @@ -13,7 +13,6 @@ import java.util.function.Consumer; 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.from.TableReference; 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 class BasicValuedPathInterpretation extends AbstractSqmPathInterpretat } } - 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 ) { diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java index 28fc5875b9..af23ff2c41 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java @@ -13,7 +13,6 @@ import java.util.function.Consumer; 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.expression.SqlTupleContainer; 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 @@ public class EmbeddableValuedPathInterpretation extends AbstractSqmPathInterp } 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( diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java index f95d7ce56c..d6b4b900b9 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java @@ -162,7 +162,9 @@ public class EntityValuedPathInterpretation extends AbstractSqmPathInterpreta // 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; diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java index 347604f0c1..6a8044bb28 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java @@ -628,8 +628,8 @@ public class SqmQuerySpec extends SqmQueryPart } 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; } } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java index 5f58d9dcb5..c735fa79af 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java @@ -52,15 +52,15 @@ public class MapIssueTest { } @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 ); } ); } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java index 0a780c4813..56f23af3cc 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/sql/exec/onetoone/bidirectional/EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest.java @@ -152,8 +152,9 @@ public class EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest { .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() );