Taewoo Kim has submitted this change and it was merged. Change subject: ASTERIXDB-1727: Fix an issue with multiple aggregates in one group-by ......................................................................
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 <[email protected]> Tested-by: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Reviewed-by: Yingyi Bu <[email protected]> --- M asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java A asterixdb/asterix-app/src/test/resources/runtimets/queries/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.query.aql A asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate/query-ASTERIXDB-1727/query-ASTERIXDB-1727.1.adm M asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml M hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java 5 files changed, 94 insertions(+), 1 deletion(-) Approvals: Yingyi Bu: Looks good to me, approved Jenkins: Verified; No violations found; Verified 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 @@ } } - 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(); 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 +} 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 } 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> 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 @@ 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(); -- To view, visit https://asterix-gerrit.ics.uci.edu/1343 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ifb5619cc3ece00ab83962d53e004f203684df9ee Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]>
