Repository: asterixdb Updated Branches: refs/heads/master e4d919e52 -> f40081f5a
ASTERIXDB-1727: Fix an issue with multiple aggregates in one group-by - Although the hash group-by hint is given, if multiple aggregate operators exist in the subplan of group-by, the physical operator of the given group-by should not be set as EXTERNAL_GROUP_BY since we don't support multiple aggregates in the external group by physical operator. Change-Id: Ifb5619cc3ece00ab83962d53e004f203684df9ee Reviewed-on: https://asterix-gerrit.ics.uci.edu/1343 Sonar-Qube: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Tested-by: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Integration-Tests: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Reviewed-by: Yingyi Bu <buyin...@gmail.com> Project: http://git-wip-us.apache.org/repos/asf/asterixdb/repo Commit: http://git-wip-us.apache.org/repos/asf/asterixdb/commit/f40081f5 Tree: http://git-wip-us.apache.org/repos/asf/asterixdb/tree/f40081f5 Diff: http://git-wip-us.apache.org/repos/asf/asterixdb/diff/f40081f5 Branch: refs/heads/master Commit: f40081f5ad0146885e9f9a416598bf46661a2f54 Parents: e4d919e Author: Taewoo Kim <wangs...@yahoo.com> Authored: Thu Nov 10 14:39:37 2016 -0800 Committer: Taewoo Kim <wangs...@yahoo.com> Committed: Thu Nov 10 17:38:45 2016 -0800 ---------------------------------------------------------------------- .../rules/SetAsterixPhysicalOperatorsRule.java | 14 ++++- .../query-ASTERIXDB-1727.1.query.aql | 64 ++++++++++++++++++++ .../query-ASTERIXDB-1727.1.adm | 2 + .../src/test/resources/runtimets/testsuite.xml | 5 ++ .../SetAlgebricksPhysicalOperatorsRule.java | 10 +++ 5 files changed, 94 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/asterixdb/blob/f40081f5/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java ---------------------------------------------------------------------- diff --git a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java index 677b9a7..473a2a5 100644 --- a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java +++ b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java @@ -139,7 +139,19 @@ public class SetAsterixPhysicalOperatorsRule implements IAlgebraicRewriteRule { } } - if (hasIntermediateAgg) { + // Check whether there are multiple aggregates in the sub plan. + // Currently, we don't support multiple aggregates in one external group-by. + boolean multipleAggOpsFound = false; + ILogicalOperator r1Logical = aggOp; + while (r1Logical.hasInputs()) { + r1Logical = r1Logical.getInputs().get(0).getValue(); + if (r1Logical.getOperatorTag() == LogicalOperatorTag.AGGREGATE) { + multipleAggOpsFound = true; + break; + } + } + + if (hasIntermediateAgg && !multipleAggOpsFound) { for (int i = 0; i < aggNum; i++) { AbstractFunctionCallExpression expr = (AbstractFunctionCallExpression) aggExprs .get(i).getValue(); http://git-wip-us.apache.org/repos/asf/asterixdb/blob/f40081f5/asterixdb/asterix-app/src/test/resources/runtimets/queries/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.query.aql ---------------------------------------------------------------------- diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.query.aql b/asterixdb/asterix-app/src/test/resources/runtimets/queries/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.query.aql new file mode 100644 index 0000000..955f820 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.query.aql @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Description : Tests that the exception in ASTERIDXDB-1727 issue is not reproduced. + * Because of the current limitation of the system (see the generateMergeAggregationExpressions + method of the SetAlgebricksPhysicalOperatorsRule class for more details.), hash hint will be + ignored. + * Success : Yes + */ + +let $customer := {{ {"cid" : 1}, {"cid" : 2} }} + +let $orders := {{ + {"oid": 100, + "ocid" : 1, + "priority" : 10, + "class" : "A", + "items" : [{"price" : 1000}, { "price" : 2000}] + }, + {"oid": 200, + "ocid" : 2, + "priority" : 20, + "class" : "A", + "items" : [{"price" : 2000}, {"price" : 3000}] + } +}} + +for $c in $customer +for $o in $orders +where + $c.cid = $o.ocid +for $i in $o.items +/*+ hash */ +group by $o_orderid := $o.oid, $o_class := $o.class, $o_priority := $o.priority + with $i +let $price := sum ( + for $t in $i + return + $t.price +) +order by $price desc, $o_class +return { + "o_orderkey": $o_orderid, + "price": $price, + "o_class": $o_class, + "o_priority": $o_priority +} http://git-wip-us.apache.org/repos/asf/asterixdb/blob/f40081f5/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.adm ---------------------------------------------------------------------- diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.adm new file mode 100644 index 0000000..46c80f8 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.adm @@ -0,0 +1,2 @@ +{ "o_orderkey": 200, "price": 5000, "o_class": "A", "o_priority": 20 } +{ "o_orderkey": 100, "price": 3000, "o_class": "A", "o_priority": 10 } http://git-wip-us.apache.org/repos/asf/asterixdb/blob/f40081f5/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml ---------------------------------------------------------------------- diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml index dd859c4..5664c72 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml @@ -613,6 +613,11 @@ </compilation-unit> </test-case> <test-case FilePath="aggregate"> + <compilation-unit name="query-ASTERIXDB-1727"> + <output-dir compare="Text">query-ASTERIXDB-1727</output-dir> + </compilation-unit> + </test-case> + <test-case FilePath="aggregate"> <compilation-unit name="group_only"> <output-dir compare="Text">group_only</output-dir> </compilation-unit> http://git-wip-us.apache.org/repos/asf/asterixdb/blob/f40081f5/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java ---------------------------------------------------------------------- diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java index 0c09fc0..5240781 100644 --- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java +++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java @@ -438,6 +438,16 @@ public class SetAlgebricksPhysicalOperatorsRule implements IAlgebraicRewriteRule if (r0Logical.getOperatorTag() != LogicalOperatorTag.AGGREGATE) { return false; } + + // Check whether there are multiple aggregates in the sub plan. + ILogicalOperator r1Logical = r0Logical; + while (r1Logical.hasInputs()) { + r1Logical = r1Logical.getInputs().get(0).getValue(); + if (r1Logical.getOperatorTag() == LogicalOperatorTag.AGGREGATE) { + return false; + } + } + AggregateOperator aggOp = (AggregateOperator) r0.getValue(); List<Mutable<ILogicalExpression>> aggFuncRefs = aggOp.getExpressions(); List<LogicalVariable> originalAggVars = aggOp.getVariables();