From eba1003c19b151ea39bc5b01564adf21c47da963 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Thu, 7 Jun 2018 00:32:39 +1200 Subject: [PATCH] #1404 - Query.findCount() with query.setDistinct(true) ... doesn't give count distinct SQL query --- .../server/core/DefaultServer.java | 7 +- .../server/core/OrmQueryRequest.java | 10 +++ .../server/query/CQueryBuilder.java | 64 ++++++++++++++----- .../ebeaninternal/server/query/SqlTree.java | 4 ++ .../server/query/SqlTreeNode.java | 4 ++ .../server/query/SqlTreeNodeBean.java | 5 ++ .../server/query/SqlTreeNodeExtraJoin.java | 5 ++ .../query/SqlTreeNodeManyWhereJoin.java | 5 ++ .../java/org/tests/query/TestRowCount.java | 51 +++++++++++++-- 9 files changed, 130 insertions(+), 25 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java index 9ad5f07ef..245673619 100644 --- a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java +++ b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java @@ -1294,8 +1294,11 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer { @Override public int findCount(Query query, Transaction t) { - SpiQuery copy = ((SpiQuery) query).copy(); - return findCountWithCopy(copy, t); + SpiQuery spiQuery = ((SpiQuery) query); + if (!spiQuery.isDistinct()) { + spiQuery = spiQuery.copy(); + } + return findCountWithCopy(spiQuery, t); } @Override diff --git a/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java b/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java index ed09fdb20..799c5b9d2 100644 --- a/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java +++ b/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java @@ -81,6 +81,8 @@ public final class OrmQueryRequest extends BeanRequest implements SpiOrmQuery private BeanPropertyAssocMany manyProperty; + private boolean inlineCountDistinct; + /** * Create the InternalQueryRequest. */ @@ -742,4 +744,12 @@ public final class OrmQueryRequest extends BeanRequest implements SpiOrmQuery public void slowQueryCheck(long executionTimeMicros, int rowCount) { ebeanServer.slowQueryCheck(executionTimeMicros, rowCount, query); } + + public void setInlineCountDistinct() { + inlineCountDistinct = true; + } + + public boolean isInlineCountDistinct() { + return inlineCountDistinct; + } } diff --git a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java index 43706cd79..cd1dc413c 100644 --- a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java +++ b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java @@ -241,10 +241,14 @@ class CQueryBuilder { ManyWhereJoins manyWhereJoins = query.getManyWhereJoins(); - if (manyWhereJoins.isFormulaWithJoin()) { - query.select(manyWhereJoins.getFormulaProperties()); - } else { - query.setSelectId(); + boolean countDistinct = query.isDistinct(); + if (!countDistinct) { + // minimise select clause for standard count + if (manyWhereJoins.isFormulaWithJoin()) { + query.select(manyWhereJoins.getFormulaProperties()); + } else { + query.setSelectId(); + } } CQueryPredicates predicates = new CQueryPredicates(binder, request); @@ -263,23 +267,35 @@ class CQueryBuilder { } boolean hasMany = sqlTree.hasMany(); - String sqlSelect = "select count(*)"; - if (hasMany) { - // need to count distinct id's ... - query.setSqlDistinct(true); - sqlSelect = null; + + String sqlSelect = null; + if (countDistinct) { + if (sqlTree.isSingleProperty()) { + request.setInlineCountDistinct(); + } + } else { + if (hasMany) { + // need to count distinct id's ... + query.setSqlDistinct(true); + } else { + sqlSelect = "select count(*)"; + } } SqlLimitResponse s = buildSql(sqlSelect, request, predicates, sqlTree); String sql = s.getSql(); - if (hasMany || query.isRawSql()) { - int pos = sql.lastIndexOf(" order by "); // remove order by - mssql does not accept order by in subqueries - if (pos != -1) { - sql = sql.substring(0, pos); - } - sql = "select count(*) from ( " + sql + ")"; - if (selectCountWithAlias) { - sql += " as c"; + + if (!request.isInlineCountDistinct()) { + if (countDistinct) { + sql = wrapSelectCount(sql); + + } else if (hasMany || query.isRawSql()) { + // remove order by - mssql does not accept order by in subqueries + int pos = sql.lastIndexOf(" order by "); + if (pos != -1) { + sql = sql.substring(0, pos); + } + sql = wrapSelectCount(sql); } } @@ -290,6 +306,14 @@ class CQueryBuilder { return new CQueryRowCount(queryPlan, request, predicates); } + private String wrapSelectCount(String sql) { + sql = "select count(*) from ( " + sql + ")"; + if (selectCountWithAlias) { + sql += " as c"; + } + return sql; + } + /** * Return the SQL Select statement as a String. Converts logical property * names to physical deployment column names. @@ -536,6 +560,9 @@ class CQueryBuilder { if (!useSqlLimiter) { sb.append("select "); if (query.isDistinctQuery()) { + if (request.isInlineCountDistinct()) { + sb.append("count("); + } sb.append("distinct "); String distinctOn = select.getDistinctOn(); if (distinctOn != null) { @@ -551,6 +578,9 @@ class CQueryBuilder { } else { sb.append(select.getSelectSql()); } + if (request.isInlineCountDistinct()) { + sb.append(")"); + } if (query.isDistinctQuery() && dbOrderBy != null && !query.isSingleAttribute()) { // add the orderBy columns to the select clause (due to distinct) sb.append(", ").append(DbOrderByTrim.trim(dbOrderBy)); diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTree.java b/src/main/java/io/ebeaninternal/server/query/SqlTree.java index d4207a9e8..b6ee3c980 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTree.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTree.java @@ -159,4 +159,8 @@ class SqlTree { boolean hasMany() { return manyProperty != null || rootNode.hasMany(); } + + boolean isSingleProperty() { + return rootNode.isSingleProperty(); + } } diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNode.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNode.java index 8406d1fe2..c503581d8 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNode.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNode.java @@ -83,4 +83,8 @@ interface SqlTreeNode { */ ScalarType getSingleAttributeScalarType(); + /** + * Return true if the query is known to only have a single property selected. + */ + boolean isSingleProperty(); } diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java index 96fd09d05..339b08a95 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java @@ -138,6 +138,11 @@ class SqlTreeNodeBean implements SqlTreeNode { return false; } + @Override + public boolean isSingleProperty() { + return properties != null && properties.length == 1 && children.length == 0; + } + @Override public ScalarType getSingleAttributeScalarType() { if (properties == null || properties.length == 0) { diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeExtraJoin.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeExtraJoin.java index 17943ddd1..b517965e4 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeExtraJoin.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeExtraJoin.java @@ -39,6 +39,11 @@ class SqlTreeNodeExtraJoin implements SqlTreeNode { this.manyJoin = assocBeanProperty instanceof STreePropertyAssocMany; } + @Override + public boolean isSingleProperty() { + return false; + } + @Override public void buildRawSqlSelectChain(List selectChain) { // nothing to add diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyWhereJoin.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyWhereJoin.java index ae0a810ab..c31ea619c 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyWhereJoin.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyWhereJoin.java @@ -36,6 +36,11 @@ class SqlTreeNodeManyWhereJoin implements SqlTreeNode { this.parentPrefix = split[0]; } + @Override + public boolean isSingleProperty() { + return false; + } + @Override public ScalarType getSingleAttributeScalarType() { throw new IllegalStateException("No expected"); diff --git a/src/test/java/org/tests/query/TestRowCount.java b/src/test/java/org/tests/query/TestRowCount.java index f75e5dc6b..ba27f2b5e 100644 --- a/src/test/java/org/tests/query/TestRowCount.java +++ b/src/test/java/org/tests/query/TestRowCount.java @@ -3,13 +3,16 @@ package org.tests.query; import io.ebean.BaseTestCase; import io.ebean.Ebean; import io.ebean.Query; +import org.junit.Test; +import org.tests.model.basic.Customer; import org.tests.model.basic.Order; import org.tests.model.basic.ResetBasicData; -import org.junit.Assert; -import org.junit.Test; import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; + public class TestRowCount extends BaseTestCase { @Test @@ -17,8 +20,12 @@ public class TestRowCount extends BaseTestCase { ResetBasicData.reset(); - Query query = Ebean.find(Order.class).fetch("details").where().gt("id", 1) - .gt("details.id", 1).order("id desc"); + Query query = Ebean.find(Order.class) + .fetch("details") + .where() + .gt("id", 1) + .gt("details.id", 1) + .order("id desc"); int rc = query.findCount(); @@ -30,8 +37,40 @@ public class TestRowCount extends BaseTestCase { order.getStatus(); } - Assert.assertEquals("same rc to ids.size() ", rc, ids.size()); - Assert.assertEquals("same rc to list.size()", rc, list.size()); + assertEquals("same rc to ids.size() ", rc, ids.size()); + assertEquals("same rc to list.size()", rc, list.size()); + } + + @Test + public void find_count_distinct_singleProperty() { + + ResetBasicData.reset(); + + Query query = Ebean.find(Customer.class) + .setDistinct(true) + .select("anniversary") + .where().eq("status", Customer.Status.NEW) + .query(); + + int count = query.findCount(); + + assertThat(sqlOf(query)).contains("select count(distinct t0.anniversary) from o_customer t0 where t0.status = ?"); + assertThat(count).isGreaterThan(0); + } + + @Test + public void find_count_distinct_multipleProperties() { + + ResetBasicData.reset(); + + Query query = Ebean.find(Customer.class) + .setDistinct(true) + .select("anniversary, status"); + + int count = query.findCount(); + + assertThat(sqlOf(query)).contains("select count(*) from ( select distinct t0.anniversary, t0.status from o_customer t0)"); + assertThat(count).isGreaterThan(0); } }