From ef155c22c1c2e57d0ce9fcc21f506efa1bba5be2 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Fri, 10 Nov 2023 10:38:55 +0100 Subject: [PATCH] HHH-17379 HHH-17397 Improve check for non-optimizable path expressions --- .../hibernate/query/sqm/internal/SqmUtil.java | 41 +++++++++++-------- .../sqm/sql/BaseSqmToSqlAstConverter.java | 10 ++--- .../BasicValuedPathInterpretation.java | 6 +-- .../EmbeddableValuedPathInterpretation.java | 6 +-- .../orm/test/jpa/ql/MapIssueTest.java | 6 +-- ...ssociationsOneOfWhichIsAJoinTableTest.java | 5 +-- 6 files changed, 36 insertions(+), 38 deletions(-) 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 98c0f85f35..24ef4dd572 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 @@ -55,6 +55,7 @@ import org.hibernate.query.sqm.tree.expression.SqmJpaCriteriaParameterWrapper; 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 class SqmUtil { ); } - 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() ) ); + && modelPartContainer.getPartMappingType() instanceof ManagedMappingType; } - private static boolean groupByClauseContains(SqmQueryPart sqmQueryPart, NavigablePath path) { - return sqmQueryPart.isSimpleQueryPart() && sqmQueryPart.getFirstQuerySpec().groupByClauseContains( path ); - } - - 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; } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java index a940d9b946..986f871e8f 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java @@ -223,7 +223,6 @@ import org.hibernate.query.sqm.tree.from.SqmEntityJoin; 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.NANOSECOND; 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 abstract class BaseSqmToSqlAstConverter extends Base 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 @@ public abstract class BaseSqmToSqlAstConverter extends Base // 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( 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 47e9cd81b5..92c21e8d95 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 @@ -82,10 +82,8 @@ public class BasicValuedPathInterpretation extends AbstractSqmPathInterpretat } 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 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 18f5bbc6d0..28fc5875b9 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 @@ -66,10 +66,8 @@ public class EmbeddableValuedPathInterpretation extends AbstractSqmPathInterp 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 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 c735fa79af..5f58d9dcb5 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 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 ); } ); } 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 56f23af3cc..0a780c4813 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,9 +152,8 @@ public class EntityWithBidirectionalAssociationsOneOfWhichIsAJoinTableTest { .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() );