From 0dc8b9eaddbd9364181040d15ece8889bd351d4f Mon Sep 17 00:00:00 2001 From: Christian Beikov Date: Fri, 3 Mar 2023 15:25:20 +0100 Subject: [PATCH] HHH-15766 Add query spec parenthesis also when order by is used within query group --- .../dialect/OracleLegacySqlAstTranslator.java | 11 ++++++++++- .../dialect/SQLServerLegacySqlAstTranslator.java | 6 ++++++ .../hibernate/dialect/OracleSqlAstTranslator.java | 11 ++++++++++- .../dialect/SQLServerSqlAstTranslator.java | 6 ++++++ .../dialect/SybaseASESqlAstTranslator.java | 15 ++------------- .../internal/util/collections/Stack.java | 5 +++++ .../internal/util/collections/StandardStack.java | 8 ++++++++ .../sql/ast/spi/AbstractSqlAstTranslator.java | 6 +++--- .../orm/test/query/hql/HqlUnionTest.java | 4 ++++ 9 files changed, 54 insertions(+), 18 deletions(-) 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 04a3f1c42f..a58561efd6 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 @@ -335,7 +335,16 @@ public class OracleLegacySqlAstTranslator extends Abstr public void visitOffsetFetchClause(QueryPart queryPart) { if ( !isRowNumberingCurrentQueryPart() ) { if ( supportsOffsetFetchClause() ) { - renderOffsetFetchClause( queryPart, true ); + if ( getQueryPartStack().depth() > 1 && queryPart.hasSortSpecifications() + && getQueryPartStack().peek( 1 ) instanceof QueryGroup + && ( queryPart.isRoot() && !hasLimit() || !queryPart.hasOffsetOrFetchClause() ) ) { + // If the current query part has a query group parent, no offset/fetch clause, but an order by clause, + // then we must render "offset 0 rows" as that is needed for the SQL to be valid + appendSql( " offset 0 rows" ); + } + else { + renderOffsetFetchClause( queryPart, true ); + } } else { assertRowsOnlyFetchClauseType( queryPart ); diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/SQLServerLegacySqlAstTranslator.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/SQLServerLegacySqlAstTranslator.java index 04406ec802..93644cfff5 100644 --- a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/SQLServerLegacySqlAstTranslator.java +++ b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/SQLServerLegacySqlAstTranslator.java @@ -323,6 +323,12 @@ public class SQLServerLegacySqlAstTranslator extends Ab else if ( offsetFetchClauseMode == OffsetFetchClauseMode.EMULATED ) { renderTopClause( querySpec, isRowsOnlyFetchClauseType( querySpec ), true ); } + else if ( getQueryPartStack().depth() > 1 && querySpec.hasSortSpecifications() + && getQueryPartStack().peek( 1 ) instanceof QueryGroup ) { + // If the current query spec has a query group parent, no offset/fetch clause, but an order by clause, + // then we must render "top 100 percent" as that is needed for the SQL to be valid + appendSql( "top 100 percent " ); + } super.visitSqlSelections( selectClause ); } 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 0aaabed383..d692ad4399 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java @@ -339,7 +339,16 @@ public class OracleSqlAstTranslator extends SqlAstTrans public void visitOffsetFetchClause(QueryPart queryPart) { if ( !isRowNumberingCurrentQueryPart() ) { if ( supportsOffsetFetchClause() ) { - renderOffsetFetchClause( queryPart, true ); + if ( getQueryPartStack().depth() > 1 && queryPart.hasSortSpecifications() + && getQueryPartStack().peek( 1 ) instanceof QueryGroup + && ( queryPart.isRoot() && !hasLimit() || !queryPart.hasOffsetOrFetchClause() ) ) { + // If the current query part has a query group parent, no offset/fetch clause, but an order by clause, + // then we must render "offset 0 rows" as that is needed for the SQL to be valid + appendSql( " offset 0 rows" ); + } + else { + renderOffsetFetchClause( queryPart, true ); + } } else { assertRowsOnlyFetchClauseType( queryPart ); diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/SQLServerSqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/dialect/SQLServerSqlAstTranslator.java index 6f819d4794..f2a6d6c555 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/SQLServerSqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/SQLServerSqlAstTranslator.java @@ -302,6 +302,12 @@ public class SQLServerSqlAstTranslator extends SqlAstTr else if ( offsetFetchClauseMode == OffsetFetchClauseMode.EMULATED ) { renderTopClause( querySpec, isRowsOnlyFetchClauseType( querySpec ), true ); } + else if ( getQueryPartStack().depth() > 1 && querySpec.hasSortSpecifications() + && getQueryPartStack().peek( 1 ) instanceof QueryGroup ) { + // If the current query spec has a query group parent, no offset/fetch clause, but an order by clause, + // then we must render "top 100 percent" as that is needed for the SQL to be valid + appendSql( "top 100 percent " ); + } super.visitSqlSelections( selectClause ); } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/SybaseASESqlAstTranslator.java b/hibernate-core/src/main/java/org/hibernate/dialect/SybaseASESqlAstTranslator.java index b57b1587fa..0dfc91292a 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/SybaseASESqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/SybaseASESqlAstTranslator.java @@ -146,9 +146,7 @@ public class SybaseASESqlAstTranslator extends Abstract @Override protected void visitSqlSelections(SelectClause selectClause) { - if ( supportsTopClause() ) { - renderTopClause( (QuerySpec) getQueryPartStack().getCurrent(), true, false ); - } + renderTopClause( (QuerySpec) getQueryPartStack().getCurrent(), true, false ); super.visitSqlSelections( selectClause ); } @@ -192,7 +190,7 @@ public class SybaseASESqlAstTranslator extends Abstract public void visitOffsetFetchClause(QueryPart queryPart) { assertRowsOnlyFetchClauseType( queryPart ); if ( !queryPart.isRoot() && queryPart.hasOffsetOrFetchClause() ) { - if ( queryPart.getFetchClauseExpression() != null && !supportsTopClause() || queryPart.getOffsetClauseExpression() != null ) { + if ( queryPart.getFetchClauseExpression() != null && queryPart.getOffsetClauseExpression() != null ) { throw new IllegalArgumentException( "Can't emulate offset fetch clause in subquery" ); } } @@ -387,11 +385,6 @@ public class SybaseASESqlAstTranslator extends Abstract return true; } - @Override - protected boolean needsMaxRows() { - return !supportsTopClause(); - } - @Override protected boolean supportsRowValueConstructorSyntax() { return false; @@ -412,10 +405,6 @@ public class SybaseASESqlAstTranslator extends Abstract return " from (select 1) dual(c1)"; } - private boolean supportsTopClause() { - return true; - } - private boolean supportsParameterOffsetFetchExpression() { return false; } diff --git a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/Stack.java b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/Stack.java index 28a30a92e3..d8b5e7b72d 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/Stack.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/Stack.java @@ -35,6 +35,11 @@ public interface Stack { */ T getCurrent(); + /** + * The element at the given offset, relative to the top of the stack + */ + T peek(int offsetFromTop); + /** * The element currently at the bottom of the stack */ diff --git a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/StandardStack.java b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/StandardStack.java index 1ce86c6f06..ba5c04b800 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/StandardStack.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/StandardStack.java @@ -72,6 +72,14 @@ public final class StandardStack implements Stack { return elements[top - 1]; } + @Override + public T peek(int offsetFromTop) { + if ( isEmpty() ) { + return null; + } + return elements[top - offsetFromTop - 1]; + } + @Override public T getRoot() { if ( isEmpty() ) { 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 3fc3f5e837..be8088e5cd 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 @@ -2992,9 +2992,9 @@ public abstract class AbstractSqlAstTranslator implemen } String queryGroupAlias = null; if ( currentQueryPart instanceof QueryGroup ) { - // We always need query wrapping if we are in a query group and this query spec has a fetch clause - // because of order by precedence in SQL - if ( querySpec.hasOffsetOrFetchClause() ) { + // We always need query wrapping if we are in a query group and this query spec has a fetch or order by + // clause, because of order by precedence in SQL + if ( querySpec.hasOffsetOrFetchClause() || querySpec.hasSortSpecifications() ) { queryGroupAlias = ""; // If the parent is a query group with a fetch clause we must use a select wrapper, // or if the database does not support simple query grouping, we must use a select wrapper diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/HqlUnionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/HqlUnionTest.java index 30e22554e6..fef6f270c7 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/HqlUnionTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/HqlUnionTest.java @@ -2,10 +2,13 @@ package org.hibernate.orm.test.query.hql; import java.util.List; +import org.hibernate.dialect.SybaseASEDialect; + import org.hibernate.testing.TestForIssue; import org.hibernate.testing.orm.junit.DomainModel; 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.BeforeAll; import org.junit.jupiter.api.Test; @@ -22,6 +25,7 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThat; ) @SessionFactory @TestForIssue(jiraKey = "HHH-15766") +@SkipForDialect(dialectClass = SybaseASEDialect.class, reason = "Sybase ASE does not support order by in subqueries") public class HqlUnionTest { @BeforeAll