diff --git a/Jenkinsfile b/Jenkinsfile index 77c8cf6c91..c9b3a3cbb7 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -60,109 +60,129 @@ stage('Build') { environments.each { BuildEnvironment buildEnv -> executions.put(buildEnv.tag, { runBuildOnNode(buildEnv.node) { + def containerName = null env.JAVA_HOME="${tool buildEnv.buildJdkTool}" env.PATH="${env.JAVA_HOME}/bin:${env.PATH}" stage('Checkout') { checkout scm } - stage('Start database') { - switch (buildEnv.dbName) { - case "mysql8": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('mysql:8.0.21').pull() - } - sh "./docker_db.sh mysql_8_0" - break; - case "mariadb": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('mariadb:10.5.8').pull() - } - sh "./docker_db.sh mariadb" - break; - case "postgresql": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('postgres:9.5').pull() - } - sh "./docker_db.sh postgresql_9_5" - break; - case "oracle": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('quillbuilduser/oracle-18-xe').pull() - } - sh "./docker_db.sh oracle" - break; - case "db2": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('ibmcom/db2:11.5.5.0').pull() - } - sh "./docker_db.sh db2" - break; - case "mssql": - docker.image('mcr.microsoft.com/mssql/server:2017-CU13').pull() - sh "./docker_db.sh mssql" - break; - case "sybase": - docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { - docker.image('nguoianphu/docker-sybase').pull() - } - sh "./docker_db.sh sybase" - break; - case "edb": - docker.withRegistry('https://containers.enterprisedb.com', 'hibernateci.containers.enterprisedb.com') { -// withCredentials([[$class: 'UsernamePasswordMultiBinding', credentialsId: 'hibernateci.containers.enterprisedb.com', -// usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD']]) { -// sh 'docker login -u "$USERNAME" -p "$PASSWORD" https://containers.enterprisedb.com' - docker.image('containers.enterprisedb.com/edb/edb-as-lite:v11').pull() - } - sh "./docker_db.sh edb" - break; - } - } - stage('Test') { - String goal; - String lockableResource; - switch (buildEnv.dbName) { - case "h2": - case "derby": - case "hsqldb": - goal = "-Pdb=${buildEnv.dbName}" - break; - case "mysql8": - goal = "-Pdb=mysql_ci" - break; - case "postgresql": - goal = "-Pdb=pgsql_ci" - break; - case "oracle": - goal = "-Pdb=oracle_ci -PexcludeTests=**.LockTest.testQueryTimeout*" - break; - case "oracle_ee": - goal = "-Pdb=oracle_jenkins" - lockableResource = 'ORACLE_RDS' - break; - case "hana": - goal = "-Pdb=hana_jenkins" - break; - case "edb": - goal = "-Pdb=edb_ci -DdbHost=localhost:5433" - break; - default: - goal = "-Pdb=${buildEnv.dbName}_ci" - break; - } - String cmd = "./gradlew check ${goal} -Plog-test-progress=true --stacktrace"; - try { - if (lockableResource == null) { - sh cmd + try { + stage('Start database') { + switch (buildEnv.dbName) { + case "mysql8": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('mysql:8.0.21').pull() + } + sh "./docker_db.sh mysql_8_0" + containerName = "mysql" + break; + case "mariadb": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('mariadb:10.5.8').pull() + } + sh "./docker_db.sh mariadb" + containerName = "mariadb" + break; + case "postgresql": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('postgres:9.5').pull() + } + sh "./docker_db.sh postgresql_9_5" + containerName = "postgres" + break; + case "oracle": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('quillbuilduser/oracle-18-xe').pull() + } + sh "./docker_db.sh oracle" + containerName = "oracle" + break; + case "db2": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('ibmcom/db2:11.5.5.0').pull() + } + sh "./docker_db.sh db2" + containerName = "db2" + break; + case "mssql": + docker.image('mcr.microsoft.com/mssql/server:2017-CU13').pull() + sh "./docker_db.sh mssql" + containerName = "mssql" + break; + case "sybase": + docker.withRegistry('https://index.docker.io/v1/', 'hibernateci.hub.docker.com') { + docker.image('nguoianphu/docker-sybase').pull() + } + sh "./docker_db.sh sybase" + containerName = "sybase" + break; + case "edb": + docker.withRegistry('https://containers.enterprisedb.com', 'hibernateci.containers.enterprisedb.com') { + // withCredentials([[$class: 'UsernamePasswordMultiBinding', credentialsId: 'hibernateci.containers.enterprisedb.com', + // usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD']]) { + // sh 'docker login -u "$USERNAME" -p "$PASSWORD" https://containers.enterprisedb.com' + docker.image('containers.enterprisedb.com/edb/edb-as-lite:v11').pull() + } + sh "./docker_db.sh edb" + containerName = "edb" + break; } - else { - lock(lockableResource) { + } + stage('Test') { + // Clean by default otherwise the PackagedEntityManager tests fail on a node that previously ran a different DB + boolean clean = true; + String goal; + String lockableResource; + switch (buildEnv.dbName) { + case "h2": + case "derby": + case "hsqldb": + goal = "-Pdb=${buildEnv.dbName}" + break; + case "mysql8": + goal = "-Pdb=mysql_ci" + break; + case "postgresql": + goal = "-Pdb=pgsql_ci" + break; + case "oracle": + goal = "-Pdb=oracle_ci -PexcludeTests=**.LockTest.testQueryTimeout*" + break; + case "oracle_ee": + goal = "-Pdb=oracle_jenkins" + lockableResource = 'ORACLE_RDS' + break; + case "hana": + // For HANA we have to also clean because this is a shared VM and the compile cache can become a problem + clean = true; + goal = "-Pdb=hana_jenkins" + break; + case "edb": + goal = "-Pdb=edb_ci -DdbHost=localhost:5433" + break; + default: + goal = "-Pdb=${buildEnv.dbName}_ci" + break; + } + String cmd = "./gradlew" + (clean ? " clean" : "") + " check ${goal} -Plog-test-progress=true --stacktrace"; + try { + if (lockableResource == null) { sh cmd } + else { + lock(lockableResource) { + sh cmd + } + } + } + finally { + junit '**/target/test-results/test/*.xml' } } - finally { - junit '**/target/test-results/test/*.xml' + } + finally { + if ( containerName != null ) { + sh "docker rm -f ${containerName}" } } } @@ -218,7 +238,7 @@ void runBuildOnNode(String label, Closure body) { node( label ) { pruneDockerContainers() try { - timeout( [time: 1, unit: 'HOURS'], body ) + timeout( [time: 90, unit: 'MINUTES'], body ) } finally { pruneDockerContainers() diff --git a/docker_db.sh b/docker_db.sh index 51ae5d8c9b..ba57df6740 100755 --- a/docker_db.sh +++ b/docker_db.sh @@ -3,16 +3,72 @@ mysql_5_7() { docker rm -f mysql || true docker run --name mysql -e MYSQL_USER=hibernate_orm_test -e MYSQL_PASSWORD=hibernate_orm_test -e MYSQL_DATABASE=hibernate_orm_test -e MYSQL_ROOT_PASSWORD=hibernate_orm_test -p3306:3306 -d mysql:5.7 --character-set-server=utf8mb4 --collation-server=utf8mb4_general_ci + # Give the container some time to start + OUTPUT= + n=0 + until [ "$n" -ge 5 ] + do + # Need to access STDERR. Thanks for the snippet https://stackoverflow.com/a/56577569/412446 + { OUTPUT="$( { docker logs mysql; } 2>&1 1>&3 3>&- )"; } 3>&1; + if [[ $OUTPUT == *"ready for connections"* ]]; then + break; + fi + n=$((n+1)) + echo "Waiting for MySQL to start..." + sleep 3 + done + if [ "$n" -ge 5 ]; then + echo "MySQL failed to start and configure after 15 seconds" + else + echo "MySQL successfully started" + fi } mysql_8_0() { docker rm -f mysql || true docker run --name mysql -e MYSQL_USER=hibernate_orm_test -e MYSQL_PASSWORD=hibernate_orm_test -e MYSQL_ROOT_PASSWORD=hibernate_orm_test -e MYSQL_DATABASE=hibernate_orm_test -e MYSQL_ROOT_PASSWORD=hibernate_orm_test -p3306:3306 -d mysql:8.0.21 --character-set-server=utf8mb4 --collation-server=utf8mb4_0900_ai_ci + # Give the container some time to start + OUTPUT= + n=0 + until [ "$n" -ge 5 ] + do + # Need to access STDERR. Thanks for the snippet https://stackoverflow.com/a/56577569/412446 + { OUTPUT="$( { docker logs mysql; } 2>&1 1>&3 3>&- )"; } 3>&1; + if [[ $OUTPUT == *"ready for connections"* ]]; then + break; + fi + n=$((n+1)) + echo "Waiting for MySQL to start..." + sleep 3 + done + if [ "$n" -ge 5 ]; then + echo "MySQL failed to start and configure after 15 seconds" + else + echo "MySQL successfully started" + fi } mariadb() { docker rm -f mariadb || true docker run --name mariadb -e MYSQL_USER=hibernate_orm_test -e MYSQL_PASSWORD=hibernate_orm_test -e MYSQL_DATABASE=hibernate_orm_test -e MYSQL_ROOT_PASSWORD=hibernate_orm_test -p3306:3306 -d mariadb:10.5.8 --character-set-server=utf8mb4 --collation-server=utf8mb4_general_ci + OUTPUT= + n=0 + until [ "$n" -ge 5 ] + do + # Need to access STDERR. Thanks for the snippet https://stackoverflow.com/a/56577569/412446 + { OUTPUT="$( { docker logs mariadb; } 2>&1 1>&3 3>&- )"; } 3>&1; + if [[ $OUTPUT == *"ready for connections"* ]]; then + break; + fi + n=$((n+1)) + echo "Waiting for MariaDB to start..." + sleep 3 + done + if [ "$n" -ge 5 ]; then + echo "MariaDB failed to start and configure after 15 seconds" + else + echo "MariaDB successfully started" + fi } postgresql_9_5() { diff --git a/gradle/databases.gradle b/gradle/databases.gradle index 6d68d2f07a..73f23c4e83 100644 --- a/gradle/databases.gradle +++ b/gradle/databases.gradle @@ -145,6 +145,13 @@ ext { 'jdbc.pass' : 'hibernate_orm_test', 'jdbc.url' : 'jdbc:oracle:thin:@hibernate-testing-oracle-se.ccuzkqo3zqzq.us-east-1.rds.amazonaws.com:1521:ORCL' ], + oracle_rds : [ + 'db.dialect' : 'org.hibernate.dialect.OracleDialect', + 'jdbc.driver': 'oracle.jdbc.OracleDriver', + 'jdbc.user' : 'hibernate_orm_test', + 'jdbc.pass' : 'hibernate_orm_test', + 'jdbc.url' : 'jdbc:oracle:thin:@localhost:1521:ORCL' + ], // Use ./docker_db.sh oracle_ee to start the database oracle_docker : [ 'db.dialect' : 'org.hibernate.dialect.OracleDialect', 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 2c0c1d3a45..5c051777bc 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/OracleSqlAstTranslator.java @@ -129,15 +129,41 @@ public class OracleSqlAstTranslator extends AbstractSql Expression fetchExpression, FetchClauseType fetchClauseType, boolean emulateFetchClause) { - if ( queryPart instanceof QuerySpec && !queryPart.hasSortSpecifications() && offsetExpression == null && fetchClauseType == FetchClauseType.ROWS_ONLY ) { + if ( queryPart instanceof QuerySpec && offsetExpression == null && fetchClauseType == FetchClauseType.ROWS_ONLY ) { // Special case for Oracle to support locking along with simple max results paging final QuerySpec querySpec = (QuerySpec) queryPart; withRowNumbering( querySpec, + false, () -> { - appendSql( "select * from (" ); + final QueryPart currentQueryPart = getQueryPartStack().getCurrent(); + final boolean needsParenthesis; + final boolean needsWrapper; + if ( currentQueryPart instanceof QueryGroup ) { + needsParenthesis = false; + // visitQuerySpec will add the select wrapper + needsWrapper = !currentQueryPart.hasOffsetOrFetchClause(); + } + else { + needsParenthesis = !querySpec.isRoot(); + needsWrapper = true; + } + if ( needsWrapper ) { + if ( needsParenthesis ) { + appendSql( '(' ); + } + appendSql( "select * from " ); + if ( !needsParenthesis ) { + appendSql( '(' ); + } + } super.visitQuerySpec( querySpec ); - appendSql( ") where rownum <= " ); + if ( needsWrapper ) { + if ( !needsParenthesis ) { + appendSql( ')' ); + } + } + appendSql( " where rownum <= " ); final Stack clauseStack = getClauseStack(); clauseStack.push( Clause.WHERE ); try { @@ -151,6 +177,12 @@ public class OracleSqlAstTranslator extends AbstractSql finally { clauseStack.pop(); } + + if ( needsWrapper ) { + if ( needsParenthesis ) { + appendSql( ')' ); + } + } } ); } @@ -165,6 +197,29 @@ public class OracleSqlAstTranslator extends AbstractSql } } + @Override + protected void visitOrderBy(List sortSpecifications) { + // If we have a query part for row numbering, there is no need to render the order by clause + // as that is part of the row numbering window function already, by which we then order by in the outer query + final QueryPart queryPartForRowNumbering = getQueryPartForRowNumbering(); + if ( queryPartForRowNumbering == null ) { + renderOrderBy( true, sortSpecifications ); + } + else { + // This logic is tightly coupled to emulateFetchOffsetWithWindowFunctions + // so that this is rendered when we end up in the special case for Oracle that renders a rownum filter + final FetchClauseType fetchClauseType = getFetchClauseTypeForRowNumbering( queryPartForRowNumbering ); + if ( fetchClauseType == FetchClauseType.ROWS_ONLY && queryPartForRowNumbering instanceof QuerySpec ) { + final QuerySpec querySpec = (QuerySpec) queryPartForRowNumbering; + if ( querySpec.getOffsetClauseExpression() == null + && ( !querySpec.isRoot() || getOffsetParameter() == null ) ) { + // When rendering `rownum` for Oracle, we need to render the order by clause still + renderOrderBy( true, sortSpecifications ); + } + } + } + } + @Override protected void visitValuesList(List valuesList) { if ( valuesList.size() < 2 ) { @@ -223,22 +278,12 @@ public class OracleSqlAstTranslator extends AbstractSql @Override protected void renderRowNumber(SelectClause selectClause, QueryPart queryPart) { - if ( supportsOffsetFetchClause() || selectClause.isDistinct() ) { - final List sortSpecifications = getSortSpecificationsRowNumbering( selectClause, queryPart ); - if ( selectClause.isDistinct() ) { - appendSql( "dense_rank()" ); - } - else { - if ( sortSpecifications.isEmpty() ) { - appendSql( "rownum" ); - return; - } - appendSql( "row_number()" ); - } - visitOverClause( Collections.emptyList(), sortSpecifications ); + if ( !queryPart.hasSortSpecifications() ) { + // Oracle doesn't allow an empty over clause for the row_number() function + appendSql( "rownum" ); } else { - appendSql( "rownum" ); + super.renderRowNumber( selectClause, queryPart ); } } 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 320ec584f1..d6666a2522 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 @@ -1644,7 +1644,25 @@ public abstract class BaseSqmToSqlAstConverter extends Base continue OUTER; } } - expressions.add( new SqlSelectionExpression( selection ) ); + if ( currentSqmQueryPart instanceof SqmQueryGroup ) { + // Reusing the SqlSelection for query groups would be wrong because the aliases do no exist + // So we have to use a literal expression in a new SqlSelection instance to refer to the position + expressions.add( + new SqlSelectionExpression( + new SqlSelectionImpl( + selection.getJdbcResultSetIndex(), + selection.getValuesArrayPosition(), + new QueryLiteral<>( + selection.getValuesArrayPosition(), + StandardBasicTypes.INTEGER + ) + ) + ) + ); + } + else { + expressions.add( new SqlSelectionExpression( selection ) ); + } } if ( expressions.size() == 1 ) { 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 3a5edd6074..7b50989363 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 @@ -127,6 +127,11 @@ public class BasicValuedPathInterpretation extends AbstractSqmPathInterpretat // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // DomainResultProducer + @Override + public Expression getSqlExpression() { + return columnReference; + } + @Override public void accept(SqlAstWalker sqlTreeWalker) { columnReference.accept( sqlTreeWalker ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/DiscriminatedAssociationPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/DiscriminatedAssociationPathInterpretation.java index 51931b6ab0..82ccdc3145 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/DiscriminatedAssociationPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/DiscriminatedAssociationPathInterpretation.java @@ -77,6 +77,11 @@ public class DiscriminatedAssociationPathInterpretation extends AbstractSqmPa this.sqlTuple = sqlTuple; } + @Override + public SqlTuple getSqlExpression() { + return sqlTuple; + } + @Override public void accept(SqlAstWalker sqlTreeWalker) { sqlTuple.accept( sqlTreeWalker ); 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 68419f2d32..04aa2aa0bb 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 @@ -86,6 +86,7 @@ public class EmbeddableValuedPathInterpretation extends AbstractSqmPathInterp this.sqlExpression = sqlExpression; } + @Override public SqlTuple getSqlExpression() { return sqlExpression; } 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 819b0c4562..5e73c47282 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 @@ -232,6 +232,11 @@ public class EntityValuedPathInterpretation extends AbstractSqmPathInterpreta this.sqlExpression = sqlExpression; } + @Override + public Expression getSqlExpression() { + return sqlExpression; + } + @Override public void accept(SqlAstWalker sqlTreeWalker) { sqlExpression.accept( sqlTreeWalker ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/NonAggregatedCompositeValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/NonAggregatedCompositeValuedPathInterpretation.java index ec80e340ec..e37caae8ae 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/NonAggregatedCompositeValuedPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/NonAggregatedCompositeValuedPathInterpretation.java @@ -58,6 +58,7 @@ public class NonAggregatedCompositeValuedPathInterpretation this.sqlExpression = sqlExpression; } + @Override public SqlTuple getSqlExpression() { return sqlExpression; } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/PluralValuedSimplePathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/PluralValuedSimplePathInterpretation.java index 4832d8e0fb..fbf42ad70c 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/PluralValuedSimplePathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/PluralValuedSimplePathInterpretation.java @@ -48,6 +48,11 @@ public class PluralValuedSimplePathInterpretation extends AbstractSqmPathInte this.sqlExpression = sqlExpression; } + @Override + public Expression getSqlExpression() { + return sqlExpression; + } + @Override public void accept(SqlAstWalker sqlTreeWalker) { throw new NotYetImplementedFor6Exception( getClass() ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/SqmPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/SqmPathInterpretation.java index c4bc4337f5..670457e048 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/SqmPathInterpretation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/SqmPathInterpretation.java @@ -24,4 +24,8 @@ public interface SqmPathInterpretation extends Expression, DomainResultProduc @Override ModelPart getExpressionType(); + + default Expression getSqlExpression() { + return this; + } } 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 737ad3ddb1..f24813b71a 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 @@ -29,6 +29,7 @@ import org.hibernate.metamodel.mapping.ModelPart; import org.hibernate.persister.entity.EntityPersister; import org.hibernate.persister.entity.Queryable; import org.hibernate.query.IllegalQueryOperationException; +import org.hibernate.query.sqm.sql.internal.SqmPathInterpretation; import org.hibernate.sql.ast.tree.cte.CteMaterialization; import org.hibernate.sql.ast.tree.cte.CteSearchClauseKind; import org.hibernate.query.FetchClauseType; @@ -154,9 +155,11 @@ import org.hibernate.sql.exec.spi.JdbcParameterBinding; import org.hibernate.sql.exec.spi.JdbcParameterBindings; import org.hibernate.sql.exec.spi.JdbcSelect; import org.hibernate.sql.exec.spi.JdbcUpdate; +import org.hibernate.sql.results.internal.SqlSelectionImpl; import org.hibernate.sql.results.jdbc.internal.JdbcValuesMappingProducerStandard; import org.hibernate.type.BasicType; import org.hibernate.type.IntegerType; +import org.hibernate.type.StandardBasicTypes; import org.hibernate.type.StringType; import org.hibernate.type.descriptor.WrapperOptions; import org.hibernate.type.descriptor.jdbc.JdbcLiteralFormatter; @@ -192,7 +195,12 @@ public abstract class AbstractSqlAstTranslator implemen private final Set affectedTableNames = new HashSet<>(); private String dmlTargetTableAlias; private boolean needsSelectAliases; + // We must reset the queryPartForRowNumbering fields to null if a query part is visited that does not + // contribute to the row numbering i.e. if the query part is a sub-query in the where clause. + // To determine whether a query part contributes to row numbering, we remember the clause depth + // and when visiting a query part, compare the current clause depth against the remembered one. private QueryPart queryPartForRowNumbering; + private int queryPartForRowNumberingClauseDepth = -1; private int queryPartForRowNumberingAliasCounter; private int queryGroupAliasCounter; private transient AbstractSqmSelfRenderingFunctionDescriptor castFunction; @@ -1360,22 +1368,51 @@ public abstract class AbstractSqlAstTranslator implemen protected void renderQueryGroup(QueryGroup queryGroup, boolean renderOrderByAndOffsetFetchClause) { final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; final boolean needsSelectAliases = this.needsSelectAliases; try { String queryGroupAlias = null; + // See the field documentation of queryPartForRowNumbering etc. for an explanation about this final QueryPart currentQueryPart = queryPartStack.getCurrent(); - if ( currentQueryPart != null && queryPartForRowNumbering != currentQueryPart ) { + if ( currentQueryPart != null && queryPartForRowNumberingClauseDepth != clauseStack.depth() ) { this.queryPartForRowNumbering = null; + this.queryPartForRowNumberingClauseDepth = -1; this.needsSelectAliases = false; } - // If we do row counting for this query group, the wrapper select is added by the caller - if ( queryPartForRowNumbering != queryGroup && !queryGroup.isRoot() ) { + // If we are row numbering the current query group, this means that we can't render the + // order by and offset fetch clause, so we must do row counting on the query group level + if ( queryPartForRowNumbering == queryGroup ) { this.needsSelectAliases = true; queryGroupAlias = "grp_" + queryGroupAliasCounter + "_"; queryGroupAliasCounter++; appendSql( "select " ); appendSql( queryGroupAlias ); - appendSql( ".* from (" ); + appendSql( ".* " ); + final SelectClause firstSelectClause = queryGroup.getFirstQuerySpec().getSelectClause(); + final List sqlSelections = firstSelectClause.getSqlSelections(); + final int sqlSelectionsSize = sqlSelections.size(); + // We need this synthetic select clause to properly render the ORDER BY within the OVER clause + // of the row numbering functions + final SelectClause syntheticSelectClause = new SelectClause( sqlSelectionsSize ); + for ( int i = 0; i < sqlSelectionsSize; i++ ) { + syntheticSelectClause.addSqlSelection( + new SqlSelectionImpl( + i + 1, + i, + new ColumnReference( + queryGroupAlias, + "c" + i, + false, + null, + null, + StandardBasicTypes.INTEGER, + null + ) + ) + ); + } + renderRowNumberingSelectItems( syntheticSelectClause, queryPartForRowNumbering ); + appendSql( " from (" ); } queryPartStack.push( queryGroup ); final List queryParts = queryGroup.getQueryParts(); @@ -1399,6 +1436,7 @@ public abstract class AbstractSqlAstTranslator implemen finally { queryPartStack.pop(); this.queryPartForRowNumbering = queryPartForRowNumbering; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; this.needsSelectAliases = needsSelectAliases; } } @@ -1406,23 +1444,29 @@ public abstract class AbstractSqlAstTranslator implemen @Override public void visitQuerySpec(QuerySpec querySpec) { final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; final boolean needsSelectAliases = this.needsSelectAliases; final ForUpdateClause forUpdate = this.forUpdate; try { this.forUpdate = null; + // See the field documentation of queryPartForRowNumbering etc. for an explanation about this + // In addition, we also reset the row numbering if the currently row numbered query part is a query group + // which means this query spec is a part of that query group. + // We want the row numbering to happen on the query group level, not on the query spec level, so we reset final QueryPart currentQueryPart = queryPartStack.getCurrent(); - if ( currentQueryPart != null && queryPartForRowNumbering != currentQueryPart ) { + if ( currentQueryPart != null && ( queryPartForRowNumbering instanceof QueryGroup || queryPartForRowNumberingClauseDepth != clauseStack.depth() ) ) { this.queryPartForRowNumbering = null; + this.queryPartForRowNumberingClauseDepth = -1; } String queryGroupAlias = ""; final boolean needsParenthesis; if ( currentQueryPart instanceof QueryGroup ) { - // We always need query wrapping if we are in a query group and the query part has a fetch clause + // 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 ( needsParenthesis = querySpec.hasOffsetOrFetchClause() ) { - // If the parent is a query group with a fetch clause, we must use an alias - // Some DBMS don't support grouping query expressions and need a select wrapper + // If the parent is a query group with a fetch clause, + // or if the database does not support simple query grouping, we must use a select wrapper if ( !supportsSimpleQueryGrouping() || currentQueryPart.hasOffsetOrFetchClause() ) { - this.needsSelectAliases = true; queryGroupAlias = " grp_" + queryGroupAliasCounter + "_"; queryGroupAliasCounter++; appendSql( "select" ); @@ -1436,7 +1480,7 @@ public abstract class AbstractSqlAstTranslator implemen } queryPartStack.push( querySpec ); if ( needsParenthesis ) { - appendSql( "(" ); + appendSql( '(' ); } visitSelectClause( querySpec.getSelectClause() ); visitFromClause( querySpec.getFromClause() ); @@ -1451,13 +1495,14 @@ public abstract class AbstractSqlAstTranslator implemen } if ( needsParenthesis ) { - appendSql( ")" ); + appendSql( ')' ); appendSql( queryGroupAlias ); } } finally { this.queryPartStack.pop(); this.queryPartForRowNumbering = queryPartForRowNumbering; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; this.needsSelectAliases = needsSelectAliases; if ( queryPartForRowNumbering == null ) { this.forUpdate = forUpdate; @@ -1485,6 +1530,12 @@ public abstract class AbstractSqlAstTranslator implemen } protected Expression resolveAliasedExpression(Expression expression) { + // This can happen when using window functions for emulating the offset/fetch clause of a query group + // But in that case we always use a SqlSelectionExpression anyway, so this is fine as it doesn't need resolving + if ( queryPartStack.getCurrent() == null ) { + assert expression instanceof SqlSelectionExpression; + return ( (SqlSelectionExpression) expression ).getSelection().getExpression(); + } return resolveAliasedExpression( queryPartStack.getCurrent().getFirstQuerySpec().getSelectClause().getSqlSelections(), expression @@ -1501,6 +1552,12 @@ public abstract class AbstractSqlAstTranslator implemen else if ( expression instanceof SqlSelectionExpression ) { return ( (SqlSelectionExpression) expression ).getSelection().getExpression(); } + else if ( expression instanceof SqmPathInterpretation ) { + final Expression sqlExpression = ( (SqmPathInterpretation) expression ).getSqlExpression(); + if ( sqlExpression instanceof SqlSelectionExpression ) { + return ( (SqlSelectionExpression) sqlExpression ).getSelection().getExpression(); + } + } return expression; } @@ -1958,7 +2015,7 @@ public abstract class AbstractSqlAstTranslator implemen } } - public void visitSortSpecification(Expression sortExpression, SortOrder sortOrder, NullPrecedence nullPrecedence) { + protected void visitSortSpecification(Expression sortExpression, SortOrder sortOrder, NullPrecedence nullPrecedence) { if ( nullPrecedence == null || nullPrecedence == NullPrecedence.NONE ) { nullPrecedence = sessionFactory.getSessionFactoryOptions().getDefaultNullPrecedence(); } @@ -2624,12 +2681,25 @@ public abstract class AbstractSqlAstTranslator implemen FetchClauseType fetchClauseType, boolean emulateFetchClause) { final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; final boolean needsSelectAliases = this.needsSelectAliases; try { this.queryPartForRowNumbering = queryPart; + this.queryPartForRowNumberingClauseDepth = clauseStack.depth(); this.needsSelectAliases = true; final String alias = "r_" + queryPartForRowNumberingAliasCounter + "_"; queryPartForRowNumberingAliasCounter++; + final boolean needsParenthesis; + if ( queryPart instanceof QueryGroup ) { + // We always need query wrapping if we are in a query group and the query part has a fetch clause + needsParenthesis = queryPart.hasOffsetOrFetchClause(); + } + else { + needsParenthesis = !queryPart.isRoot(); + } + if ( needsParenthesis && !queryPart.isRoot() ) { + appendSql( '(' ); + } appendSql( "select " ); if ( getClauseStack().isEmpty() ) { appendSql( "*" ); @@ -2645,15 +2715,21 @@ public abstract class AbstractSqlAstTranslator implemen separator = COMA_SEPARATOR; } } - appendSql( " from (" ); + appendSql( " from " ); + if ( !needsParenthesis || queryPart.isRoot() ) { + appendSql( '(' ); + } queryPart.accept( this ); - appendSql( ") "); + if ( !needsParenthesis || queryPart.isRoot() ) { + appendSql( ')' ); + } + appendSql( ' '); appendSql( alias ); appendSql( " where " ); final Stack clauseStack = getClauseStack(); clauseStack.push( Clause.WHERE ); try { - if ( emulateFetchClause ) { + if ( emulateFetchClause && fetchExpression != null ) { switch ( fetchClauseType ) { case PERCENT_ONLY: appendSql( alias ); @@ -2671,6 +2747,10 @@ public abstract class AbstractSqlAstTranslator implemen case ROWS_ONLY: appendSql( alias ); appendSql( ".rn <= " ); + if ( offsetExpression != null ) { + offsetExpression.accept( this ); + appendSql( " + " ); + } fetchExpression.accept( this ); break; case PERCENT_WITH_TIES: @@ -2689,37 +2769,45 @@ public abstract class AbstractSqlAstTranslator implemen case ROWS_WITH_TIES: appendSql( alias ); appendSql( ".rnk <= " ); + if ( offsetExpression != null ) { + offsetExpression.accept( this ); + appendSql( " + " ); + } fetchExpression.accept( this ); break; } } // todo: not sure if databases handle order by row number or the original ordering better.. if ( offsetExpression == null ) { - switch ( fetchClauseType ) { - case PERCENT_ONLY: - case ROWS_ONLY: - appendSql( " order by " ); - appendSql( alias ); - appendSql( ".rn" ); - break; - case PERCENT_WITH_TIES: - case ROWS_WITH_TIES: - appendSql( " order by " ); - appendSql( alias ); - appendSql( ".rnk" ); - break; + if ( queryPart.isRoot() ) { + switch ( fetchClauseType ) { + case PERCENT_ONLY: + case ROWS_ONLY: + appendSql( " order by " ); + appendSql( alias ); + appendSql( ".rn" ); + break; + case PERCENT_WITH_TIES: + case ROWS_WITH_TIES: + appendSql( " order by " ); + appendSql( alias ); + appendSql( ".rnk" ); + break; + } } } else { - if ( emulateFetchClause ) { + if ( emulateFetchClause && fetchExpression != null ) { appendSql( " and " ); } appendSql( alias ); appendSql( ".rn > " ); offsetExpression.accept( this ); - appendSql( " order by " ); - appendSql( alias ); - appendSql( ".rn" ); + if ( queryPart.isRoot() ) { + appendSql( " order by " ); + appendSql( alias ); + appendSql( ".rn" ); + } } // We render the FOR UPDATE clause in the outer query @@ -2732,24 +2820,31 @@ public abstract class AbstractSqlAstTranslator implemen finally { clauseStack.pop(); } + if ( needsParenthesis && !queryPart.isRoot() ) { + appendSql( ')' ); + } } finally { this.queryPartForRowNumbering = queryPartForRowNumbering; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; this.needsSelectAliases = needsSelectAliases; } } - protected final void withRowNumbering(QueryPart queryPart, Runnable r) { + protected final void withRowNumbering(QueryPart queryPart, boolean needsSelectAliases, Runnable r) { final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; - final boolean needsSelectAliases = this.needsSelectAliases; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; + final boolean originalNeedsSelectAliases = this.needsSelectAliases; try { this.queryPartForRowNumbering = queryPart; - this.needsSelectAliases = false; + this.queryPartForRowNumberingClauseDepth = clauseStack.depth(); + this.needsSelectAliases = needsSelectAliases; r.run(); } finally { this.queryPartForRowNumbering = queryPartForRowNumbering; - this.needsSelectAliases = needsSelectAliases; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; + this.needsSelectAliases = originalNeedsSelectAliases; } } @@ -2787,37 +2882,7 @@ public abstract class AbstractSqlAstTranslator implemen separator = COMA_SEPARATOR; } if ( queryPartForRowNumbering != null ) { - final FetchClauseType fetchClauseType = getFetchClauseTypeForRowNumbering( queryPartForRowNumbering ); - if ( fetchClauseType != null ) { - appendSql( separator ); - switch ( fetchClauseType ) { - case PERCENT_ONLY: - appendSql( "count(*) over () cnt," ); - case ROWS_ONLY: - renderRowNumber( selectClause, queryPartForRowNumbering ); - appendSql( " rn" ); - break; - case PERCENT_WITH_TIES: - appendSql( "count(*) over () cnt," ); - case ROWS_WITH_TIES: - if ( queryPartForRowNumbering.getOffsetClauseExpression() != null ) { - renderRowNumber( selectClause, queryPartForRowNumbering ); - appendSql( " rn, " ); - } - if ( selectClause.isDistinct() ) { - appendSql( "dense_rank()" ); - } - else { - appendSql( "rank()" ); - } - visitOverClause( - Collections.emptyList(), - getSortSpecificationsRowNumbering( selectClause, queryPartForRowNumbering ) - ); - appendSql( " rnk" ); - break; - } - } + renderRowNumberingSelectItems( selectClause, queryPartForRowNumbering ); } } else { @@ -2831,6 +2896,40 @@ public abstract class AbstractSqlAstTranslator implemen } } + protected void renderRowNumberingSelectItems(SelectClause selectClause, QueryPart queryPart) { + final FetchClauseType fetchClauseType = getFetchClauseTypeForRowNumbering( queryPart ); + if ( fetchClauseType != null ) { + appendSql( COMA_SEPARATOR ); + switch ( fetchClauseType ) { + case PERCENT_ONLY: + appendSql( "count(*) over () cnt," ); + case ROWS_ONLY: + renderRowNumber( selectClause, queryPart ); + appendSql( " rn" ); + break; + case PERCENT_WITH_TIES: + appendSql( "count(*) over () cnt," ); + case ROWS_WITH_TIES: + if ( queryPart.getOffsetClauseExpression() != null ) { + renderRowNumber( selectClause, queryPart ); + appendSql( " rn, " ); + } + if ( selectClause.isDistinct() ) { + appendSql( "dense_rank()" ); + } + else { + appendSql( "rank()" ); + } + visitOverClause( + Collections.emptyList(), + getSortSpecificationsRowNumbering( selectClause, queryPart ) + ); + appendSql( " rnk" ); + break; + } + } + } + protected FetchClauseType getFetchClauseTypeForRowNumbering(QueryPart queryPartForRowNumbering) { if ( queryPartForRowNumbering.isRoot() && hasLimit() ) { return FetchClauseType.ROWS_ONLY; @@ -2912,6 +3011,39 @@ public abstract class AbstractSqlAstTranslator implemen return sortSpecificationsRowNumbering; } } + else if ( queryPart instanceof QueryGroup ) { + // When the sort specifications come from a query group which uses positional references + // we have to resolve to the actual selection expressions + final int specificationsSize = sortSpecifications.size(); + final List sortSpecificationsRowNumbering = new ArrayList<>( specificationsSize ); + final List sqlSelections = selectClause.getSqlSelections(); + for ( int i = 0; i < specificationsSize; i++ ) { + final SortSpecification sortSpecification = sortSpecifications.get( i ); + final int position; + if ( sortSpecification.getSortExpression() instanceof SqlSelectionExpression ) { + position = ( (SqlSelectionExpression) sortSpecification.getSortExpression() ) + .getSelection() + .getValuesArrayPosition(); + } + else { + assert sortSpecification.getSortExpression() instanceof QueryLiteral; + final QueryLiteral queryLiteral = (QueryLiteral) sortSpecification.getSortExpression(); + assert queryLiteral.getLiteralValue() instanceof Integer; + position = (Integer) queryLiteral.getLiteralValue(); + } + sortSpecificationsRowNumbering.add( + new SortSpecification( + new SqlSelectionExpression( + sqlSelections.get( position ) + ), + null, + sortSpecification.getSortOrder(), + sortSpecification.getNullPrecedence() + ) + ); + } + return sortSpecificationsRowNumbering; + } else { return sortSpecifications; } @@ -4187,9 +4319,11 @@ public abstract class AbstractSqlAstTranslator implemen } final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; final boolean needsSelectAliases = this.needsSelectAliases; try { this.queryPartForRowNumbering = null; + this.queryPartForRowNumberingClauseDepth = -1; this.needsSelectAliases = false; queryPartStack.push( subQuery ); appendSql( "exists (select 1" ); @@ -4242,11 +4376,12 @@ public abstract class AbstractSqlAstTranslator implemen } } - appendSql( ")" ); + appendSql( ')' ); } finally { queryPartStack.pop(); this.queryPartForRowNumbering = queryPartForRowNumbering; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; this.needsSelectAliases = needsSelectAliases; } } @@ -4279,12 +4414,14 @@ public abstract class AbstractSqlAstTranslator implemen appendSql( " " ); final QueryPart queryPartForRowNumbering = this.queryPartForRowNumbering; + final int queryPartForRowNumberingClauseDepth = this.queryPartForRowNumberingClauseDepth; final boolean needsSelectAliases = this.needsSelectAliases; try { this.queryPartForRowNumbering = null; + this.queryPartForRowNumberingClauseDepth = -1; this.needsSelectAliases = false; queryPartStack.push( subQuery ); - appendSql( "(" ); + appendSql( '(' ); visitSelectClause( subQuery.getSelectClause() ); visitFromClause( subQuery.getFromClause() ); visitWhereClause( subQuery ); @@ -4309,11 +4446,12 @@ public abstract class AbstractSqlAstTranslator implemen appendSql( order ); } renderFetch( ONE_LITERAL, null, FetchClauseType.ROWS_ONLY ); - appendSql( ")" ); + appendSql( ')' ); } finally { queryPartStack.pop(); this.queryPartForRowNumbering = queryPartForRowNumbering; + this.queryPartForRowNumberingClauseDepth = queryPartForRowNumberingClauseDepth; this.needsSelectAliases = needsSelectAliases; } } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectClause.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectClause.java index e1a7a2c6b6..45931efd9b 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectClause.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/select/SelectClause.java @@ -23,7 +23,15 @@ import org.hibernate.query.sqm.sql.internal.DomainResultProducer; public class SelectClause implements SqlAstNode { private boolean distinct; - private final List sqlSelections = new ArrayList<>(); + private final List sqlSelections; + + public SelectClause() { + this.sqlSelections = new ArrayList<>(); + } + + public SelectClause(int estimateSelectionSize) { + this.sqlSelections = new ArrayList<>( estimateSelectionSize ); + } public void makeDistinct(boolean distinct) { this.distinct = distinct;