From 63bddb4d54fececcbc43a87a7fe93545bc604689 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Wed, 11 Sep 2019 23:38:12 +1200 Subject: [PATCH] #1810 - Whan paging over query with @Aggregation ( grouping by) wrong total count is calculated --- .../server/deploy/BeanDescriptor.java | 23 ++++++++- .../deploy/meta/DeployBeanPropertyLists.java | 10 +++- .../server/query/CQueryBuilder.java | 28 ++++++----- .../java/org/tests/iud/TestCarWheelIud.java | 48 ++++++++++++++++++- .../aggregation/TestAggregationTopLevel.java | 22 +++++++++ .../java/org/tests/model/carwheel/Car.java | 33 +++++++++++++ 6 files changed, 149 insertions(+), 15 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java index de4bf14e6..8e43f8099 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java @@ -66,7 +66,6 @@ import io.ebeaninternal.server.el.ElPropertyChainBuilder; import io.ebeaninternal.server.el.ElPropertyDeploy; import io.ebeaninternal.server.el.ElPropertyValue; import io.ebeaninternal.server.persist.DeleteMode; -import io.ebeaninternal.server.persist.DmlUtil; import io.ebeaninternal.server.query.CQueryPlan; import io.ebeaninternal.server.query.ExtraJoin; import io.ebeaninternal.server.query.STreeProperty; @@ -77,6 +76,7 @@ import io.ebeaninternal.server.query.STreeType; import io.ebeaninternal.server.query.SqlBeanLoad; import io.ebeaninternal.server.querydefn.DefaultOrmQuery; import io.ebeaninternal.server.querydefn.OrmQueryDetail; +import io.ebeaninternal.server.querydefn.OrmQueryProperties; import io.ebeaninternal.server.rawsql.SpiRawSql; import io.ebeaninternal.server.type.DataBind; import io.ebeaninternal.server.type.ScalarType; @@ -333,6 +333,7 @@ public class BeanDescriptor implements BeanType, STreeType { * list of properties that are Lists/Sets/Maps (Derived). */ private final BeanProperty[] propertiesNonMany; + private final BeanProperty[] propertiesAggregate; private final BeanPropertyAssocMany[] propertiesMany; private final BeanPropertyAssocMany[] propertiesManySave; private final BeanPropertyAssocMany[] propertiesManyDelete; @@ -515,6 +516,7 @@ public class BeanDescriptor implements BeanType, STreeType { this.propertiesMany = listHelper.getMany(); this.propertiesNonMany = listHelper.getNonMany(); + this.propertiesAggregate = listHelper.getAggregates(); this.propertiesManySave = listHelper.getManySave(); this.propertiesManyDelete = listHelper.getManyDelete(); this.propertiesManyToMany = listHelper.getManyToMany(); @@ -3169,6 +3171,25 @@ public class BeanDescriptor implements BeanType, STreeType { return propertiesEmbedded; } + /** + * Return true if the query detail includes an aggregation property. + */ + public boolean includesAggregation(OrmQueryDetail detail) { + return detail != null && propertiesAggregate.length > 0 && includesAggregation(detail.getChunk(null, false)); + } + + private boolean includesAggregation(OrmQueryProperties rootProps) { + if (rootProps != null) { + final Set included = rootProps.getIncluded(); + for (BeanProperty property : propertiesAggregate) { + if (included.contains(property.getName())) { + return true; + } + } + } + return false; + } + /** * Set all properties to be loaded (recurse to embedded beans). */ diff --git a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanPropertyLists.java b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanPropertyLists.java index 276e9821c..f6280b260 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanPropertyLists.java +++ b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanPropertyLists.java @@ -10,7 +10,6 @@ import io.ebeaninternal.server.deploy.BeanPropertyIdClass; import io.ebeaninternal.server.deploy.BeanPropertyOrderColumn; import io.ebeaninternal.server.deploy.BeanPropertySimpleCollection; import io.ebeaninternal.server.deploy.InheritInfo; -import io.ebeaninternal.server.deploy.TableJoin; import io.ebeaninternal.server.deploy.generatedproperty.GeneratedProperty; import io.ebeaninternal.server.properties.BeanPropertySetter; import io.ebeaninternal.server.type.ScalarTypeString; @@ -54,6 +53,8 @@ public class DeployBeanPropertyLists { private final List nonManys = new ArrayList<>(); + private final List aggs = new ArrayList<>(); + private final List> ones = new ArrayList<>(); private final List> onesImported = new ArrayList<>(); @@ -232,6 +233,9 @@ public class DeployBeanPropertyLists { } else { nonManys.add(prop); + if (prop.isAggregation()) { + aggs.add(prop); + } if (prop.isTenantId()) { tenant = prop; } @@ -331,6 +335,10 @@ public class DeployBeanPropertyLists { return nonManys.toArray(new BeanProperty[0]); } + public BeanProperty[] getAggregates() { + return aggs.toArray(new BeanProperty[0]); + } + public BeanPropertyAssocMany[] getMany() { return manys.toArray(new BeanPropertyAssocMany[0]); } diff --git a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java index b83140706..b75ea391e 100644 --- a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java +++ b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java @@ -14,7 +14,6 @@ import io.ebean.event.readaudit.ReadAuditQueryPlan; import io.ebean.text.PathProperties; import io.ebean.util.SplitName; import io.ebean.util.StringHelper; -import io.ebeaninternal.api.ManyWhereJoins; import io.ebeaninternal.api.SpiQuery; import io.ebeaninternal.server.core.OrmQueryRequest; import io.ebeaninternal.server.deploy.BeanDescriptor; @@ -44,9 +43,6 @@ import java.util.List; */ class CQueryBuilder { - private static final String DELETE = "Delete"; - private static final String UPDATE = "Update"; - final String tableAliasPlaceHolder; final String columnAliasPrefix; @@ -104,7 +100,6 @@ class CQueryBuilder { */ CQueryUpdate buildUpdateQuery(boolean deleteRequest, OrmQueryRequest request) { - String type = (deleteRequest) ? DELETE : UPDATE; SpiQuery query = request.getQuery(); String rootTableAlias = query.getAlias(); query.setDelete(); @@ -263,12 +258,14 @@ class CQueryBuilder { query.setFirstRow(0); query.setMaxRows(0); - ManyWhereJoins manyWhereJoins = query.getManyWhereJoins(); - boolean countDistinct = query.isDistinct(); + boolean withAgg = false; if (!countDistinct) { - // minimise select clause for standard count - query.setSelectId(); + withAgg = includesAggregation(request, query); + if (!withAgg) { + // minimise select clause for standard count + query.setSelectId(); + } } CQueryPredicates predicates = new CQueryPredicates(binder, request); @@ -286,14 +283,14 @@ class CQueryBuilder { sqlTree.addSoftDeletePredicate(query); } - boolean hasMany = sqlTree.hasMany(); + boolean wrap = sqlTree.hasMany() || withAgg; String sqlSelect = null; if (countDistinct) { if (sqlTree.isSingleProperty()) { request.setInlineCountDistinct(); } - } else if (!hasMany) { + } else if (!wrap) { sqlSelect = "select count(*)"; } @@ -304,7 +301,7 @@ class CQueryBuilder { if (countDistinct) { sql = wrapSelectCount(sql); - } else if (hasMany || query.isRawSql()) { + } else if (wrap || query.isRawSql()) { // remove order by - mssql does not accept order by in subqueries int pos = sql.lastIndexOf(" order by "); if (pos != -1) { @@ -321,6 +318,13 @@ class CQueryBuilder { return new CQueryRowCount(queryPlan, request, predicates); } + /** + * Return true if the query includes an aggregation property. + */ + private boolean includesAggregation(OrmQueryRequest request, SpiQuery query) { + return request.getBeanDescriptor().includesAggregation(query.getDetail()); + } + private String wrapSelectCount(String sql) { sql = "select count(*) from ( " + sql + ")"; if (selectCountWithAlias) { diff --git a/src/test/java/org/tests/iud/TestCarWheelIud.java b/src/test/java/org/tests/iud/TestCarWheelIud.java index 621260342..442d96ba8 100644 --- a/src/test/java/org/tests/iud/TestCarWheelIud.java +++ b/src/test/java/org/tests/iud/TestCarWheelIud.java @@ -1,15 +1,21 @@ package org.tests.iud; import io.ebean.BaseTestCase; +import io.ebean.DB; import io.ebean.Ebean; +import io.ebean.PagedList; +import org.ebeantest.LoggedSqlCollector; +import org.junit.Test; import org.tests.model.carwheel.Car; import org.tests.model.carwheel.Tire; import org.tests.model.carwheel.Wheel; -import org.junit.Test; import java.util.ArrayList; import java.util.List; +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + public class TestCarWheelIud extends BaseTestCase { @Test @@ -53,4 +59,44 @@ public class TestCarWheelIud extends BaseTestCase { Ebean.delete(car2); } + + @Test + public void aggregatePaging() { + + DB.find(Car.class).delete(); + + final Car car0 = createCar("Ford", 40); + final Car car1 = createCar("Ford", 50); + final Car car2 = createCar("Mazda", 12); + + DB.saveAll(asList(car0, car1, car2)); + + LoggedSqlCollector.start(); + + final PagedList pagedList = DB.find(Car.class) + .select("brand, totalSold") + .setMaxRows(10) + .findPagedList(); + + final List list = pagedList.getList(); + final int count = pagedList.getTotalCount(); + + assertThat(list).hasSize(2); + assertThat(count).isEqualTo(2); + + final List sql = LoggedSqlCollector.stop(); + assertThat(sql).hasSize(2); + if (isH2() || isPostgres()) { + assertThat(sql.get(0)).contains("select t0.brand, sum(t0.sold) from sa_car t0 group by t0.brand limit 10"); + assertThat(sql.get(1)).contains("select count(*) from ( select t0.brand, sum(t0.sold) from sa_car t0 group by t0.brand)"); + } + + } + + private Car createCar(String brand, int sold) { + Car car0 = new Car(); + car0.setBrand(brand); + car0.setSold(sold); + return car0; + } } diff --git a/src/test/java/org/tests/model/aggregation/TestAggregationTopLevel.java b/src/test/java/org/tests/model/aggregation/TestAggregationTopLevel.java index 946ea750b..2039caa15 100644 --- a/src/test/java/org/tests/model/aggregation/TestAggregationTopLevel.java +++ b/src/test/java/org/tests/model/aggregation/TestAggregationTopLevel.java @@ -33,6 +33,28 @@ public class TestAggregationTopLevel extends BaseTestCase { assertThat(result).isNotEmpty(); } + @Test + public void query_count() { + + Query query = DB.find(DMachineStatsAgg.class) + .select("date, totalKms") + .setMaxRows(10) + .having().gt("totalKms", 1) + .query(); + + LoggedSqlCollector.start(); + query.findCount(); + query.findList(); + + List sql = LoggedSqlCollector.stop(); + assertThat(sql).hasSize(2); + + if (isH2() || isPostgres()) { + assertThat(sql.get(0)).contains("select count(*) from ( select t0.date, sum(t0.total_kms) from d_machine_stats t0 group by t0.date having sum(t0.total_kms) > ?)"); + assertThat(sql.get(1)).contains("select t0.date, sum(t0.total_kms) from d_machine_stats t0 group by t0.date having sum(t0.total_kms) > ? limit 10"); + } + } + @Test public void query_machineTotalKms_withHaving() { diff --git a/src/test/java/org/tests/model/carwheel/Car.java b/src/test/java/org/tests/model/carwheel/Car.java index 2288639bc..65981f46b 100644 --- a/src/test/java/org/tests/model/carwheel/Car.java +++ b/src/test/java/org/tests/model/carwheel/Car.java @@ -1,5 +1,7 @@ package org.tests.model.carwheel; +import io.ebean.annotation.Aggregation; + import javax.persistence.CascadeType; import javax.persistence.Entity; import javax.persistence.GeneratedValue; @@ -21,6 +23,13 @@ public class Car { @Version private int version; + private String brand; + + private int sold; + + @Aggregation("sum(sold)") + private int totalSold; + @OneToMany(mappedBy = "car", cascade = CascadeType.ALL) private List wheels; @@ -40,6 +49,30 @@ public class Car { this.version = version; } + public String getBrand() { + return brand; + } + + public void setBrand(String brand) { + this.brand = brand; + } + + public int getSold() { + return sold; + } + + public void setSold(int sold) { + this.sold = sold; + } + + public int getTotalSold() { + return totalSold; + } + + public void setTotalSold(int totalSold) { + this.totalSold = totalSold; + } + public List getWheels() { return wheels; }