From fe2a72497db442ab3e8c0f163d8d00b8cbb3382a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Victor=20NO=C3=8BL?= Date: Mon, 19 May 2025 11:54:57 +0200 Subject: [PATCH] HHH-19331: query plan cache should take parameter type into accound for cast --- .../sql/ast/spi/AbstractSqlAstTranslator.java | 11 ++- .../exec/spi/AbstractJdbcOperationQuery.java | 31 +++++++-- .../exec/spi/JdbcOperationQuerySelect.java | 69 +++++++++---------- .../test/query/hql/QueryPlanCachingTest.java | 61 ++++++++++++++-- 4 files changed, 122 insertions(+), 50 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java index 7fb0ba09e150..4f08403c9898 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java @@ -5804,10 +5804,9 @@ protected void renderCasted(Expression expression) { arguments.add( expression ); final CastTarget castTarget; if ( expression instanceof SqlTypedMappingJdbcParameter ) { - final SqlTypedMappingJdbcParameter parameter = (SqlTypedMappingJdbcParameter) expression; - final SqlTypedMapping sqlTypedMapping = parameter.getSqlTypedMapping(); + final SqlTypedMapping sqlTypedMapping = ( (SqlTypedMappingJdbcParameter) expression ).getSqlTypedMapping(); castTarget = new CastTarget( - parameter.getJdbcMapping(), + sqlTypedMapping.getJdbcMapping(), sqlTypedMapping.getColumnDefinition(), sqlTypedMapping.getLength(), sqlTypedMapping.getTemporalPrecision() != null @@ -5820,6 +5819,12 @@ protected void renderCasted(Expression expression) { castTarget = new CastTarget( expression.getExpressionType().getSingleJdbcMapping() ); } arguments.add( castTarget ); + if ( expression instanceof JdbcParameter ) { + // the value itself is not important, but its type, + // to improve performances, we could store that information + // and use it in JdbcOperationQuery.isCompatibleWith instead + addAppliedParameterBinding( (JdbcParameter) expression, null ); + } castFunction().render( this, arguments, (ReturnableType) castTarget.getJdbcMapping(), this ); } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/AbstractJdbcOperationQuery.java b/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/AbstractJdbcOperationQuery.java index b99c3e863d6c..7604f8326075 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/AbstractJdbcOperationQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/AbstractJdbcOperationQuery.java @@ -110,16 +110,37 @@ public boolean isCompatibleWith(JdbcParameterBindings jdbcParameterBindings, Que return false; } for ( Map.Entry entry : appliedParameters.entrySet() ) { - final JdbcParameterBinding binding = jdbcParameterBindings.getBinding( entry.getKey() ); + final JdbcParameter parameter = entry.getKey(); + final JdbcParameterBinding binding = jdbcParameterBindings.getBinding( parameter ); final JdbcParameterBinding appliedBinding = entry.getValue(); - //noinspection unchecked - if ( binding == null || !appliedBinding.getBindType() - .getJavaTypeDescriptor() - .areEqual( binding.getBindValue(), appliedBinding.getBindValue() ) ) { + if ( !isCompatible( parameter, appliedBinding, binding, queryOptions ) ) { return false; } } } return true; } + + protected boolean isCompatible( + JdbcParameter parameter, + JdbcParameterBinding appliedBinding, + JdbcParameterBinding binding, + QueryOptions queryOptions) { + // This is a special case where the rendered SQL depends on the presence of the parameter, + // but not specifically on the value. Some example of such situation are when generating + // SQL that depends on the type of the parameter (e.g., cast). See also HHH-19331 + // and QueryPlanCachingTest, as well as subclass overrides of this method. + if ( appliedBinding == null ) { + // We could optionally optimize this by identifying only the type and checking it in the same + // way we check the value below. + return false; + } + //noinspection unchecked + if ( binding == null || !appliedBinding.getBindType() + .getJavaTypeDescriptor() + .areEqual( binding.getBindValue(), appliedBinding.getBindValue() ) ) { + return false; + } + return true; + } } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcOperationQuerySelect.java b/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcOperationQuerySelect.java index 94870452f5c3..cd00ae9a6fb8 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcOperationQuerySelect.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcOperationQuerySelect.java @@ -148,43 +148,8 @@ public JdbcLockStrategy getLockStrategy() { @Override public boolean isCompatibleWith(JdbcParameterBindings jdbcParameterBindings, QueryOptions queryOptions) { - if ( !appliedParameters.isEmpty() ) { - if ( jdbcParameterBindings == null ) { - return false; - } - for ( Map.Entry entry : appliedParameters.entrySet() ) { - final JdbcParameter parameter = entry.getKey(); - final JdbcParameterBinding appliedBinding = entry.getValue(); - // This is a special case where the rendered SQL depends on the presence of the parameter, - // but not specifically on the value. In this case we have to re-generate the SQL if we can't find a binding - // The need for this can be tested with the OracleFollowOnLockingTest#testPessimisticLockWithMaxResultsThenNoFollowOnLocking - // Since the Limit is not part of the query plan cache key, but this has an effect on follow on locking, - // we must treat the absence of Limit parameters, when they were considered for locking, as incompatible - if ( appliedBinding == null ) { - if ( parameter == offsetParameter ) { - if ( queryOptions.getLimit() == null || queryOptions.getLimit().getFirstRowJpa() == 0 ) { - return false; - } - } - else if ( parameter == limitParameter ) { - if ( queryOptions.getLimit() == null || queryOptions.getLimit().getMaxRowsJpa() == Integer.MAX_VALUE ) { - return false; - } - } - else if ( jdbcParameterBindings.getBinding( parameter ) == null ) { - return false; - } - } - // We handle limit and offset parameters below - if ( parameter != offsetParameter && parameter != limitParameter ) { - final JdbcParameterBinding binding = jdbcParameterBindings.getBinding( parameter ); - if ( binding == null || !appliedBinding.getBindType() - .getJavaTypeDescriptor() - .areEqual( binding.getBindValue(), appliedBinding.getBindValue() ) ) { - return false; - } - } - } + if ( !super.isCompatibleWith( jdbcParameterBindings, queryOptions ) ) { + return false; } final Limit limit = queryOptions.getLimit(); if ( offsetParameter == null && limitParameter == null ) { @@ -201,6 +166,36 @@ else if ( jdbcParameterBindings.getBinding( parameter ) == null ) { return true; } + @Override + protected boolean isCompatible( + JdbcParameter parameter, + JdbcParameterBinding appliedBinding, + JdbcParameterBinding binding, + QueryOptions queryOptions) { + // This is a special case where the rendered SQL depends on the presence of the parameter, + // but not specifically on the value. In this case we have to re-generate the SQL if we can't find a binding + // The need for this can be tested with the OracleFollowOnLockingTest#testPessimisticLockWithMaxResultsThenNoFollowOnLocking + // Since the Limit is not part of the query plan cache key, but this has an effect on follow on locking, + // we must treat the absence of Limit parameters, when they were considered for locking, as incompatible. + if ( appliedBinding == null ) { + if ( parameter == offsetParameter ) { + if ( queryOptions.getLimit() == null || queryOptions.getLimit().getFirstRowJpa() == 0 ) { + return false; + } + } + else if ( parameter == limitParameter ) { + if ( queryOptions.getLimit() == null || queryOptions.getLimit().getMaxRowsJpa() == Integer.MAX_VALUE ) { + return false; + } + } + } + // We handle limit and offset parameters above and in isCompatibleWith + if ( parameter != offsetParameter && parameter != limitParameter ) { + return super.isCompatible( parameter, appliedBinding, binding, queryOptions ); + } + return true; + } + private boolean isCompatible(JdbcParameter parameter, Integer requestedValue, int defaultValue) { if ( parameter == null ) { return requestedValue == null; diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/QueryPlanCachingTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/QueryPlanCachingTest.java index ddc3a5862a28..3e42b1b7f773 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/QueryPlanCachingTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/QueryPlanCachingTest.java @@ -6,28 +6,79 @@ */ package org.hibernate.orm.test.query.hql; -import org.hibernate.orm.test.mapping.SmokeTests; +import java.math.BigDecimal; +import org.hibernate.testing.orm.domain.gambit.BasicEntity; import org.hibernate.testing.orm.junit.DomainModel; +import org.hibernate.testing.orm.junit.Jira; import org.hibernate.testing.orm.junit.ServiceRegistry; import org.hibernate.testing.orm.junit.SessionFactory; import org.hibernate.testing.orm.junit.SessionFactoryScope; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; + /** * @author Steve Ebersole */ -@DomainModel( annotatedClasses = SmokeTests.SimpleEntity.class ) +@DomainModel(annotatedClasses = BasicEntity.class) @ServiceRegistry -@SessionFactory( exportSchema = true ) +@SessionFactory(exportSchema = true) public class QueryPlanCachingTest { + + @BeforeAll + public void setUp(SessionFactoryScope scope) { + scope.inTransaction( session -> session.persist( new BasicEntity( 1, "entity_1" ) ) ); + } + + @AfterAll + public void tearDown(SessionFactoryScope scope) { + scope.getSessionFactory().getSchemaManager().truncateMappedObjects(); + } + @Test public void testHqlTranslationCaching(SessionFactoryScope scope) { scope.inTransaction( session -> { - session.createQuery( "select e from SimpleEntity e" ).list(); - session.createQuery( "select e from SimpleEntity e" ).list(); + session.createQuery( "select e from BasicEntity e" ).list(); + session.createQuery( "select e from BasicEntity e" ).list(); } ); } + + @Test + @Jira("https://hibernate.atlassian.net/browse/HHH-19331") + public void hhh19331_selectionquery(SessionFactoryScope scope) { + scope.inTransaction( session -> { + assertThat( + session.createSelectionQuery( "select :p0 from BasicEntity", Object[].class ) + .setParameter( "p0", 1 ) + .getSingleResult() + ).containsExactly( 1 ); + assertThat( + session.createSelectionQuery( "select :p0 from BasicEntity", Object[].class ) + .setParameter( "p0", BigDecimal.valueOf( 3.14 ) ) + .getSingleResult() + ).containsExactly( BigDecimal.valueOf( 3.14 ) ); + } ); + } + + @Test + @Jira("https://hibernate.atlassian.net/browse/HHH-19331") + public void hhh19331_query(SessionFactoryScope scope) { + scope.inTransaction( session -> { + assertThat( + session.createQuery( "select :p0 from BasicEntity", Object[].class ) + .setParameter( "p0", 1 ) + .getSingleResult() + ).containsExactly( 1 ); + assertThat( + session.createQuery( "select :p0 from BasicEntity", Object[].class ) + .setParameter( "p0", BigDecimal.valueOf( 3.14 ) ) + .getSingleResult() + ).containsExactly( BigDecimal.valueOf( 3.14 ) ); + } ); + } }