From 21e79125e601160101f1f27e1aea026ede9c36a9 Mon Sep 17 00:00:00 2001 From: Jan-Willem Gmelig Meyling Date: Wed, 4 Sep 2019 04:45:58 +0200 Subject: [PATCH] HHH-9301 - Support select variable refs in group by for DBMS dialects that support it While not strictly compliant with the SQL specification, the MySQL, PostgreSQL and H2 support the use of select aliases in the GROUP BY clause. An obvious benefit is that produced SQL queries will be easier to read, because complex select expressions from aggregrate queries will have to be included in the group by clause as well. These can now simply reference the aliases of the respective columns for the tuple element. However, there is also a functional difference. For function invocations that have parameterized arguments, the query optimizer can't guarantee the that the function result between the selection projection and grouping process are equal. This results in an error because a value is projected which is not grouped by. An example where this for example becomes relevant, is when parameterizing TimeScaleDB's [`time_bucket_gapfill()`](https://docs.timescale.com/latest/api#time_bucket_gapfill-examples) function. For example, using `time_bucket_gapfill()` one might want to produce a criteria query that produces the following SQL: ```sql SELECT time_bucket_gapfill(?, time, ?, ?) AS ts, avg(tg) as tg FROM iaqmeasurement GROUP BY ts ORDER BY ts; ``` When the alias is not used as grouping value, the query will yield an error: ```sql SELECT time_bucket_gapfill(?, time, ?, ?) AS ts, avg(tg) as tg FROM iaqmeasurement GROUP BY time_bucket_gapfill(?, time, ?, ?) ORDER BY ts; ``` Of course the parameter values can just be stored in a CTE as well, but I think we should consider support for variable refs in group by for DBMS dialects that support it. This pull request implements the feature equivalently to how its currently done for order by clauses, and only enables the feature for known supported dialects (H2, PostgreSQL and MySQL - based on [https://stackoverflow.com/a/3841804/2104280](https://stackoverflow.com/a/3841804/2104280)). Jira: https://hibernate.atlassian.net/browse/HHH-9301 Co-authored-by: Sayra Ranjha --- hibernate-core/src/main/antlr/hql-sql.g | 6 +- .../java/org/hibernate/dialect/Dialect.java | 5 + .../java/org/hibernate/dialect/H2Dialect.java | 6 + .../org/hibernate/dialect/MySQLDialect.java | 6 + .../dialect/PostgreSQL81Dialect.java | 6 + .../hql/internal/ast/HqlSqlWalker.java | 12 + .../org/hibernate/query/GroupByAliasTest.java | 228 ++++++++++++++++++ .../org/hibernate/testing/DialectChecks.java | 6 + 8 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/query/GroupByAliasTest.java diff --git a/hibernate-core/src/main/antlr/hql-sql.g b/hibernate-core/src/main/antlr/hql-sql.g index 5265979d5e..96c40bbbea 100644 --- a/hibernate-core/src/main/antlr/hql-sql.g +++ b/hibernate-core/src/main/antlr/hql-sql.g @@ -237,6 +237,10 @@ tokens return false; } + protected boolean isGroupExpressionResultVariableRef(AST ident) throws SemanticException { + return false; + } + protected void handleResultVariableRef(AST resultVariableRef) throws SemanticException { } @@ -394,7 +398,7 @@ resultVariableRef! ; groupClause - : #(GROUP { handleClauseStart( GROUP ); } (expr [ null ])+ ( #(HAVING logicalExpr) )? ) { + : #(GROUP { handleClauseStart( GROUP ); } ({ isGroupExpressionResultVariableRef( _t ) }? resultVariableRef | expr [ null ])+ ( #(HAVING logicalExpr) )? ) { handleClauseEnd(); } ; diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java index 385647a858..0c9563775b 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java @@ -3048,4 +3048,9 @@ public abstract class Dialect implements ConversionContext { protected String prependComment(String sql, String comment) { return "/* " + comment + " */ " + sql; } + + public boolean supportsSelectAliasInGroupByClause() { + return false; + } + } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/H2Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/H2Dialect.java index 06468ef8ee..f36701b32c 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/H2Dialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/H2Dialect.java @@ -450,4 +450,10 @@ public class H2Dialect extends Dialect { public String getQueryHintString(String query, String hints) { return IndexQueryHintHandler.INSTANCE.addQueryHints( query, hints ); } + + @Override + public boolean supportsSelectAliasInGroupByClause() { + return true; + } + } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/MySQLDialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/MySQLDialect.java index 3555c06f18..b671118133 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/MySQLDialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/MySQLDialect.java @@ -604,4 +604,10 @@ public class MySQLDialect extends Dialect { protected String escapeLiteral(String literal) { return ESCAPE_PATTERN.matcher( super.escapeLiteral( literal ) ).replaceAll( ESCAPE_PATTERN_REPLACEMENT ); } + + @Override + public boolean supportsSelectAliasInGroupByClause() { + return true; + } + } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/PostgreSQL81Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/PostgreSQL81Dialect.java index 4792de5252..a7b28147d0 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/PostgreSQL81Dialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/PostgreSQL81Dialect.java @@ -643,4 +643,10 @@ public class PostgreSQL81Dialect extends Dialect { public boolean supportsJdbcConnectionLobCreation(DatabaseMetaData databaseMetaData) { return false; } + + @Override + public boolean supportsSelectAliasInGroupByClause() { + return true; + } + } diff --git a/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/HqlSqlWalker.java b/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/HqlSqlWalker.java index eff9592a8f..d2e2c56dff 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/HqlSqlWalker.java +++ b/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/HqlSqlWalker.java @@ -1267,6 +1267,18 @@ public class HqlSqlWalker extends HqlSqlBaseWalker implements ErrorReporter, Par return false; } + @Override + protected boolean isGroupExpressionResultVariableRef(AST groupExpressionNode) throws SemanticException { + // Aliases are not sensible in subqueries + if ( getDialect().supportsSelectAliasInGroupByClause() && + !isSubQuery() && + groupExpressionNode.getType() == IDENT && + selectExpressionsByResultVariable.containsKey( groupExpressionNode.getText() ) ) { + return true; + } + return false; + } + @Override protected void handleResultVariableRef(AST resultVariableRef) throws SemanticException { if ( isSubQuery() ) { diff --git a/hibernate-core/src/test/java/org/hibernate/query/GroupByAliasTest.java b/hibernate-core/src/test/java/org/hibernate/query/GroupByAliasTest.java new file mode 100644 index 0000000000..b5e79c0325 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/query/GroupByAliasTest.java @@ -0,0 +1,228 @@ +/* + * 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 . + */ +package org.hibernate.query; + +import org.hibernate.jpa.test.BaseEntityManagerFunctionalTestCase; +import org.hibernate.testing.DialectChecks; +import org.hibernate.testing.RequiresDialectFeature; +import org.hibernate.testing.jdbc.SQLStatementInterceptor; +import org.junit.Test; + +import javax.persistence.CascadeType; +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.IdClass; +import javax.persistence.ManyToOne; +import javax.persistence.Tuple; +import java.io.Serializable; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import static org.hibernate.testing.transaction.TransactionUtil.doInJPA; +import static org.junit.Assert.assertNotNull; + +/** + * @author Jan-Willem Gmelig Meyling + * @author Sayra Ranjha + */ +@RequiresDialectFeature(value = DialectChecks.SupportsSelectAliasInGroupByClause.class, jiraKey = "HHH-9301") +public class GroupByAliasTest extends BaseEntityManagerFunctionalTestCase { + + public static final int MAX_COUNT = 15; + + private SQLStatementInterceptor sqlStatementInterceptor; + + @Override + protected void addConfigOptions(Map options) { + sqlStatementInterceptor = new SQLStatementInterceptor( options ); + } + + @Override + public Class[] getAnnotatedClasses() { + return new Class[] { + Person.class, + Association.class + }; + } + + @Override + protected void afterEntityManagerFactoryBuilt() { + doInJPA( this::entityManagerFactory, entityManager -> { + for ( int i = 0; i < MAX_COUNT; i++ ) { + Association association = new Association(); + association.setId( i ); + association.setName(String.format( "Association nr %d", i ) ); + + Person person = new Person(); + person.setId( i ); + person.setName( String.format( "Person nr %d", i ) ); + person.setAssociation(association); + person.setAge(5); + entityManager.persist( person ); + } + } ); + } + + @Test + public void testSingleIdAlias() { + sqlStatementInterceptor.clear(); + + List list = doInJPA(this::entityManagerFactory, entityManager -> { + return entityManager.createQuery( + "select p.id as id_alias, sum(p.age) " + + "from Person p group by id_alias order by id_alias", Tuple.class) + .getResultList(); + }); + + String s = sqlStatementInterceptor.getSqlQueries().get(0); + assertNotNull(s); + } + + @Test + public void testCompoundIdAlias() { + sqlStatementInterceptor.clear(); + + List list = doInJPA(this::entityManagerFactory, entityManager -> { + return entityManager.createQuery( + "select p.association as id_alias, sum(p.age) " + + "from Person p group by id_alias order by id_alias", Tuple.class) + .getResultList(); + }); + + String s = sqlStatementInterceptor.getSqlQueries().get(0); + assertNotNull(s); + } + + + @Test + public void testMultiIdAlias() { + sqlStatementInterceptor.clear(); + + List list = doInJPA(this::entityManagerFactory, entityManager -> { + return entityManager.createQuery( + "select p.id as id_alias_1, p.association as id_alias_2, sum(p.age) " + + "from Person p group by id_alias_1, id_alias_2 order by id_alias_1, id_alias_2 ", Tuple.class) + .getResultList(); + }); + + String s = sqlStatementInterceptor.getSqlQueries().get(0); + assertNotNull(s); + } + + @Entity(name = "Person") + public static class Person { + + @Id + private Integer id; + + private String name; + + private Integer age; + + @ManyToOne(cascade = CascadeType.PERSIST) + private Association association; + + public Integer getId() { + return id; + } + + public void setId(Integer id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public Integer getAge() { + return age; + } + + public void setAge(Integer age) { + this.age = age; + } + + public Association getAssociation() { + return association; + } + + public void setAssociation(Association association) { + this.association = association; + } + } + + + @IdClass(Association.IdClass.class) + @Entity(name = "Association") + public static class Association { + + public static class IdClass implements Serializable { + private Integer id; + private String name; + + public Integer getId() { + return id; + } + + public void setId(Integer id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + IdClass id1 = (IdClass) o; + return Objects.equals(id, id1.id) && + Objects.equals(name, id1.name); + } + + @Override + public int hashCode() { + return Objects.hash(id, name); + } + } + + @Id + private Integer id; + + @Id + private String name; + + + public Integer getId() { + return id; + } + + public void setId(Integer id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + } + +} diff --git a/hibernate-testing/src/main/java/org/hibernate/testing/DialectChecks.java b/hibernate-testing/src/main/java/org/hibernate/testing/DialectChecks.java index cf5ad6e365..6d08043f86 100644 --- a/hibernate-testing/src/main/java/org/hibernate/testing/DialectChecks.java +++ b/hibernate-testing/src/main/java/org/hibernate/testing/DialectChecks.java @@ -266,6 +266,12 @@ abstract public class DialectChecks { } } + public static class SupportsSelectAliasInGroupByClause implements DialectCheck { + public boolean isMatch(Dialect dialect) { + return dialect.supportsSelectAliasInGroupByClause(); + } + } + public static class SupportsNClob implements DialectCheck { @Override public boolean isMatch(Dialect dialect) {