From fa89e3e5fab433fe262832c3fd94be9b9027f731 Mon Sep 17 00:00:00 2001 From: Christian Beikov Date: Wed, 5 Oct 2022 14:45:18 +0200 Subject: [PATCH] HHH-15531 Use dense_rank instead of row_number when query uses distinct --- .../dialect/OracleLegacySqlAstTranslator.java | 7 +- .../dialect/OracleSqlAstTranslator.java | 7 +- .../internal/SqmInsertStrategyHelper.java | 98 +++++++++++++++++++ .../internal/cte/CteInsertHandler.java | 33 ++----- .../temptable/TableBasedInsertHandler.java | 17 +--- .../sqm/sql/BaseSqmToSqlAstConverter.java | 15 +-- .../orm/test/query/hql/InsertSelectTests.java | 81 ++++++++++++++- 7 files changed, 200 insertions(+), 58 deletions(-) create mode 100644 hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/SqmInsertStrategyHelper.java diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/OracleLegacySqlAstTranslator.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/OracleLegacySqlAstTranslator.java index 24a47f9d1d..01345f5138 100644 --- a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/OracleLegacySqlAstTranslator.java +++ b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/OracleLegacySqlAstTranslator.java @@ -329,6 +329,8 @@ public class OracleLegacySqlAstTranslator extends Abstr protected void renderRowNumber(SelectClause selectClause, QueryPart queryPart) { if ( !queryPart.hasSortSpecifications() ) { // Oracle doesn't allow an empty over clause for the row_number() function + // For regular window function usage, we render a constant order by, + // but since this is used for emulating limit/offset anyway, this is fine appendSql( "rownum" ); } else { @@ -344,8 +346,9 @@ public class OracleLegacySqlAstTranslator extends Abstr && over.getStartKind() == FrameKind.UNBOUNDED_PRECEDING && over.getEndKind() == FrameKind.CURRENT_ROW && over.getExclusion() == FrameExclusion.NO_OTHERS ) { - // Oracle doesn't allow an empty over clause for the row_number() function - append( "rownum" ); + // Oracle doesn't allow an empty over clause for the row_number() function, + // so we order by a constant + append( "row_number() over(order by 1)" ); return; } } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java index 4bca807be3..0f4f97ad3e 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java @@ -329,6 +329,8 @@ public class OracleSqlAstTranslator extends AbstractSql protected void renderRowNumber(SelectClause selectClause, QueryPart queryPart) { if ( !queryPart.hasSortSpecifications() ) { // Oracle doesn't allow an empty over clause for the row_number() function + // For regular window function usage, we render a constant order by, + // but since this is used for emulating limit/offset anyway, this is fine appendSql( "rownum" ); } else { @@ -344,8 +346,9 @@ public class OracleSqlAstTranslator extends AbstractSql && over.getStartKind() == FrameKind.UNBOUNDED_PRECEDING && over.getEndKind() == FrameKind.CURRENT_ROW && over.getExclusion() == FrameExclusion.NO_OTHERS ) { - // Oracle doesn't allow an empty over clause for the row_number() function - append( "rownum" ); + // Oracle doesn't allow an empty over clause for the row_number() function, + // so we order by a constant + append( "row_number() over(order by 1)" ); return; } } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/SqmInsertStrategyHelper.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/SqmInsertStrategyHelper.java new file mode 100644 index 0000000000..bc8a0989b6 --- /dev/null +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/SqmInsertStrategyHelper.java @@ -0,0 +1,98 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later + * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html + */ +package org.hibernate.query.sqm.mutation.internal; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.hibernate.engine.spi.SessionFactoryImplementor; +import org.hibernate.query.sqm.SortOrder; +import org.hibernate.query.sqm.function.SelfRenderingWindowFunctionSqlAstExpression; +import org.hibernate.sql.ast.spi.SqlSelection; +import org.hibernate.sql.ast.tree.expression.Expression; +import org.hibernate.sql.ast.tree.expression.Over; +import org.hibernate.sql.ast.tree.expression.SqlSelectionExpression; +import org.hibernate.sql.ast.tree.select.QuerySpec; +import org.hibernate.sql.ast.tree.select.SortSpecification; +import org.hibernate.type.BasicType; + +/** + * @author Christian Beikov + */ +public final class SqmInsertStrategyHelper { + + private SqmInsertStrategyHelper() { + } + + /** + * Creates a row numbering expression, that can be added to the select clause of the query spec. + */ + public static Expression createRowNumberingExpression( + QuerySpec querySpec, + SessionFactoryImplementor sessionFactory) { + final BasicType resultType = sessionFactory.getTypeConfiguration() + .getBasicTypeForJavaType( Integer.class ); + final Expression functionExpression; + final List orderList; + if ( querySpec.getSelectClause().isDistinct() ) { + assert sessionFactory.getJdbcServices().getDialect().supportsWindowFunctions(); + + functionExpression = new SelfRenderingWindowFunctionSqlAstExpression( + "dense_rank", + (appender, args, walker) -> appender.appendSql( "dense_rank()" ), + Collections.emptyList(), + null, + null, + null, + resultType, + resultType + ); + final List sortSpecifications = querySpec.getSortSpecifications(); + final List sqlSelections = querySpec.getSelectClause().getSqlSelections(); + if ( sortSpecifications == null ) { + orderList = new ArrayList<>( sqlSelections.size() ); + } + else { + orderList = new ArrayList<>( sortSpecifications.size() + sqlSelections.size() ); + orderList.addAll( sortSpecifications ); + } + for ( SqlSelection sqlSelection : sqlSelections ) { + if ( containsSelectionExpression( orderList, sqlSelection ) ) { + continue; + } + orderList.add( new SortSpecification( sqlSelection.getExpression(), SortOrder.ASCENDING ) ); + } + } + else { + functionExpression = new SelfRenderingWindowFunctionSqlAstExpression( + "row_number", + (appender, args, walker) -> appender.appendSql( "row_number()" ), + Collections.emptyList(), + null, + null, + null, + resultType, + resultType + ); + orderList = Collections.emptyList(); + } + return new Over<>( functionExpression, Collections.emptyList(), orderList ); + } + + private static boolean containsSelectionExpression(List orderList, SqlSelection sqlSelection) { + final Expression expression = sqlSelection.getExpression(); + for ( SortSpecification sortSpecification : orderList ) { + final Expression sortExpression = sortSpecification.getSortExpression(); + if ( sortExpression == expression || sortExpression instanceof SqlSelectionExpression + && ( (SqlSelectionExpression) sortExpression ).getSelection() == sqlSelection ) { + return true; + } + } + return false; + } +} diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java index e8b9de6db6..40f3239584 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java @@ -39,6 +39,7 @@ import org.hibernate.persister.entity.EntityPersister; import org.hibernate.persister.entity.Joinable; import org.hibernate.query.sqm.BinaryArithmeticOperator; import org.hibernate.query.sqm.ComparisonOperator; +import org.hibernate.query.sqm.mutation.internal.SqmInsertStrategyHelper; import org.hibernate.spi.NavigablePath; import org.hibernate.query.SemanticException; import org.hibernate.query.sqm.SortOrder; @@ -267,23 +268,13 @@ public class CteInsertHandler implements InsertHandler { ); } if ( !assignsId && entityDescriptor.getIdentifierGenerator() instanceof PostInsertIdentifierGenerator ) { - final BasicType rowNumberType = sessionFactory.getTypeConfiguration() - .getBasicTypeForJavaType( Integer.class ); querySpec.getSelectClause().addSqlSelection( new SqlSelectionImpl( 1, 0, - new Over<>( - new SelfRenderingFunctionSqlAstExpression( - "row_number", - (appender, args, walker) -> appender.appendSql( - "row_number()" ), - Collections.emptyList(), - rowNumberType, - rowNumberType - ), - Collections.emptyList(), - Collections.emptyList() + SqmInsertStrategyHelper.createRowNumberingExpression( + querySpec, + sessionFactory ) ) ); @@ -1000,23 +991,13 @@ public class CteInsertHandler implements InsertHandler { idColumnReference ) ); - final BasicType rowNumberType = sessionFactory.getTypeConfiguration() - .getBasicTypeForJavaType( Integer.class ); finalResultQuery.getSelectClause().addSqlSelection( new SqlSelectionImpl( 1, 0, - new Over<>( - new SelfRenderingFunctionSqlAstExpression( - "row_number", - (appender, args, walker) -> appender.appendSql( - "row_number()" ), - Collections.emptyList(), - rowNumberType, - rowNumberType - ), - Collections.emptyList(), - Collections.emptyList() + SqmInsertStrategyHelper.createRowNumberingExpression( + querySpec, + sessionFactory ) ) ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java index 028c1ff1a3..a6894c6e7b 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/TableBasedInsertHandler.java @@ -31,6 +31,7 @@ import org.hibernate.query.sqm.internal.DomainParameterXref; import org.hibernate.query.sqm.internal.SqmJdbcExecutionContextAdapter; import org.hibernate.query.sqm.mutation.internal.InsertHandler; import org.hibernate.query.sqm.mutation.internal.MultiTableSqmMutationConverter; +import org.hibernate.query.sqm.mutation.internal.SqmInsertStrategyHelper; import org.hibernate.query.sqm.sql.BaseSqmToSqlAstConverter; import org.hibernate.query.sqm.tree.expression.SqmParameter; import org.hibernate.query.sqm.tree.insert.SqmInsertSelectStatement; @@ -216,23 +217,13 @@ public class TableBasedInsertHandler implements InsertHandler { ); insertStatement.getTargetColumnReferences().add( columnReference ); targetPathColumns.add( new Assignment( columnReference, columnReference ) ); - final BasicType rowNumberType = sessionFactory.getTypeConfiguration() - .getBasicTypeForJavaType( Integer.class ); querySpec.getSelectClause().addSqlSelection( new SqlSelectionImpl( 1, 0, - new Over<>( - new SelfRenderingFunctionSqlAstExpression( - "row_number", - (appender, args, walker) -> appender.appendSql( - "row_number()" ), - Collections.emptyList(), - rowNumberType, - rowNumberType - ), - Collections.emptyList(), - Collections.emptyList() + SqmInsertStrategyHelper.createRowNumberingExpression( + querySpec, + sessionFactory ) ) ); 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 edca40771d..f0406467a2 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 @@ -136,6 +136,7 @@ import org.hibernate.query.sqm.function.SelfRenderingAggregateFunctionSqlAstExpr import org.hibernate.query.sqm.function.SelfRenderingFunctionSqlAstExpression; import org.hibernate.query.sqm.internal.DomainParameterXref; import org.hibernate.query.sqm.internal.SqmMappingModelHelper; +import org.hibernate.query.sqm.mutation.internal.SqmInsertStrategyHelper; import org.hibernate.query.sqm.produce.function.internal.PatternRenderer; import org.hibernate.query.sqm.spi.BaseSemanticQueryWalker; import org.hibernate.query.sqm.sql.internal.BasicValuedPathInterpretation; @@ -1402,22 +1403,10 @@ public abstract class BaseSqmToSqlAstConverter extends Base if ( !sessionFactory.getJdbcServices().getDialect().supportsWindowFunctions() ) { return false; } - final BasicType rowNumberType = sessionFactory.getTypeConfiguration() - .getBasicTypeForJavaType( Integer.class ); identifierSelection = new SqlSelectionImpl( 1, 0, - new Over<>( - new SelfRenderingFunctionSqlAstExpression( - "row_number", - (appender, args, walker) -> appender.appendSql( "row_number()" ), - Collections.emptyList(), - rowNumberType, - rowNumberType - ), - Collections.emptyList(), - Collections.emptyList() - ) + SqmInsertStrategyHelper.createRowNumberingExpression( querySpec, sessionFactory ) ); selectClause.addSqlSelection( identifierSelection ); return true; diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/InsertSelectTests.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/InsertSelectTests.java index 7a68da68a0..0fe86db503 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/InsertSelectTests.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/InsertSelectTests.java @@ -6,25 +6,52 @@ */ package org.hibernate.orm.test.query.hql; +import org.hibernate.dialect.DerbyDialect; + import org.hibernate.testing.TestForIssue; import org.hibernate.testing.jdbc.SQLStatementInspector; import org.hibernate.testing.orm.junit.DomainModel; import org.hibernate.testing.orm.junit.ServiceRegistry; import org.hibernate.testing.orm.junit.SessionFactory; import org.hibernate.testing.orm.junit.SessionFactoryScope; +import org.hibernate.testing.orm.junit.SkipForDialect; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import jakarta.persistence.Entity; import jakarta.persistence.GeneratedValue; import jakarta.persistence.Id; +import static org.junit.jupiter.api.Assertions.assertEquals; + @ServiceRegistry @DomainModel( annotatedClasses = { - InsertSelectTests.EntityEntry.class + InsertSelectTests.EntityEntry.class, + InsertSelectTests.EntitySource.class }) @SessionFactory(statementInspectorClass = SQLStatementInspector.class) public class InsertSelectTests { + @BeforeEach + public void prepareTestData(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + session.persist( new EntitySource( "A" ) ); + session.persist( new EntitySource( "A" ) ); + } + ); + } + + @AfterEach + public void cleanupTestData(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + session.createMutationQuery( "delete from java.lang.Object" ).executeUpdate(); + } + ); + } + @Test @TestForIssue( jiraKey = "HHH-15527") public void testInsertSelectGeneratedAssigned(SessionFactoryScope scope) { @@ -32,12 +59,47 @@ public class InsertSelectTests { scope.inTransaction( session -> { statementInspector.clear(); - session.createMutationQuery("insert into EntityEntry (id, name) select 1, 'abc' from EntityEntry e").executeUpdate(); + session.createMutationQuery( + "insert into EntityEntry (id, name) " + + "select 1, 'abc' from EntityEntry e" + ).executeUpdate(); statementInspector.assertExecutedCount( 1 ); } ); } + @Test + @TestForIssue( jiraKey = "HHH-15531") + @SkipForDialect(dialectClass = DerbyDialect.class, reason = "Derby doesn't really support window functions, " + + "but this requires the use of a dense_rank window function. We could emulate this, but don't think it's worth it") + public void testInsertSelectDistinct(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + final int rows = session.createMutationQuery( + "insert into EntityEntry (name) " + + "select distinct e.name from EntitySource e" + ).executeUpdate(); + assertEquals( 1, rows ); + } + ); + } + + @Test + @TestForIssue( jiraKey = "HHH-15531") + @SkipForDialect(dialectClass = DerbyDialect.class, reason = "Derby doesn't really support window functions and " + + "its attempt at a row_number function fails to deliver the desired semantics") + public void testInsertSelectGroupBy(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + final int rows = session.createMutationQuery( + "insert into EntityEntry (name) " + + "select e.name from EntitySource e group by e.name" + ).executeUpdate(); + assertEquals( 1, rows ); + } + ); + } + @Entity(name = "EntityEntry") public static class EntityEntry { @Id @@ -45,4 +107,19 @@ public class InsertSelectTests { Integer id; String name; } + + @Entity(name = "EntitySource") + public static class EntitySource { + @Id + @GeneratedValue + Integer id; + String name; + + public EntitySource() { + } + + public EntitySource(String name) { + this.name = name; + } + } }