This is an automated email from the ASF dual-hosted git repository. mblow pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/asterixdb.git
commit bdc5c05a79a3da6a1dc95cf8877470c224bfe79f Author: Ali Alsuliman <[email protected]> AuthorDate: Tue Feb 18 14:24:27 2020 -0800 [ASTERIXDB-2700][*DB][RT] Fix invalid plan caused by order-by operator in a subquery - user model changes: no - storage format changes: no - interface changes: no Details: Instead of removing a redundant sort operator from the plan, replace it with an empty assign operator to avoid potential connection of a sort-merge connector to another connector. - modified RemoveUnusedAssignAndAggregateRule to excluded necessary empty assign operators from being removed. Change-Id: I8bc11fa046cb15fab04057086817bd400b7809c0 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/5103 Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> Reviewed-by: Dmitry Lychagin <[email protected]> --- .../optimizerts/queries/query-ASTERIXDB-2700.sqlpp | 40 ++++++++++++++++++++++ .../optimizerts/results/query-ASTERIXDB-2700.plan | 35 +++++++++++++++++++ .../query-ASTERIXDB-2700.1.ddl.sqlpp | 35 +++++++++++++++++++ .../query-ASTERIXDB-2700.2.update.sqlpp | 39 +++++++++++++++++++++ .../query-ASTERIXSB-2700.3.query.sqlpp | 30 ++++++++++++++++ .../query-ASTERIXDB-2700.3.adm | 8 +++++ .../test/resources/runtimets/testsuite_sqlpp.xml | 5 +++ .../rules/EnforceStructuralPropertiesRule.java | 26 +++++++------- .../rules/RemoveUnusedAssignAndAggregateRule.java | 40 ++++++++++++++++++---- 9 files changed, 239 insertions(+), 19 deletions(-) diff --git a/asterixdb/asterix-app/src/test/resources/optimizerts/queries/query-ASTERIXDB-2700.sqlpp b/asterixdb/asterix-app/src/test/resources/optimizerts/queries/query-ASTERIXDB-2700.sqlpp new file mode 100644 index 0000000..80ada37 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/optimizerts/queries/query-ASTERIXDB-2700.sqlpp @@ -0,0 +1,40 @@ +/* + * 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. + */ + +DROP DATAVERSE bigfun IF EXISTS; +CREATE DATAVERSE bigfun; +USE bigfun; + +CREATE TYPE GleambookUserType AS { gb: int32, id: string }; +CREATE TYPE GleambookMessageType AS { gb: int32, message_id: string }; + +CREATE DATASET GleambookUsersComposite(GleambookUserType) PRIMARY KEY gb,id; +CREATE DATASET GleambookMessagesComposite(GleambookMessageType) PRIMARY KEY gb,message_id; + +CREATE INDEX usrSinceIx ON GleambookUsersComposite(user_since: string); +CREATE INDEX authorIdIx ON GleambookMessagesComposite(author_id: string); + +SET `compiler.sort.parallel` "false"; + +FROM (SELECT VALUE u + FROM GleambookUsersComposite u + WHERE u.user_since >= '2008-07-22T00:00:00' + ORDER BY u.id) AS user, GleambookMessagesComposite AS msg +WHERE msg.author_id /*+ indexnl */ = user.id +SELECT user.name AS uname, msg.message AS message; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/optimizerts/results/query-ASTERIXDB-2700.plan b/asterixdb/asterix-app/src/test/resources/optimizerts/results/query-ASTERIXDB-2700.plan new file mode 100644 index 0000000..3cd606b --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/optimizerts/results/query-ASTERIXDB-2700.plan @@ -0,0 +1,35 @@ +-- DISTRIBUTE_RESULT |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- ASSIGN |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- STREAM_SELECT |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- BTREE_SEARCH |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STABLE_SORT [$$56(ASC), $$57(ASC)] |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- BTREE_SEARCH |PARTITIONED| + -- BROADCAST_EXCHANGE |PARTITIONED| + -- ASSIGN |PARTITIONED| + -- SORT_MERGE_EXCHANGE [$$42(ASC) ] |PARTITIONED| + -- STABLE_SORT [$$42(ASC)] |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- STREAM_SELECT |PARTITIONED| + -- ASSIGN |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- BTREE_SEARCH |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STABLE_SORT [$$53(ASC), $$54(ASC)] |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- STREAM_PROJECT |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- BTREE_SEARCH |PARTITIONED| + -- ONE_TO_ONE_EXCHANGE |PARTITIONED| + -- ASSIGN |PARTITIONED| + -- EMPTY_TUPLE_SOURCE |PARTITIONED| \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.1.ddl.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.1.ddl.sqlpp new file mode 100644 index 0000000..4572164 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.1.ddl.sqlpp @@ -0,0 +1,35 @@ +/* + * 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: This test case is to verify the fix for ASTERIXDB-2700 + */ + +DROP DATAVERSE bigfun IF EXISTS; +CREATE DATAVERSE bigfun; +USE bigfun; + +CREATE TYPE GleambookUserType AS { gb: int32, id: string }; +CREATE TYPE GleambookMessageType AS { gb: int32, message_id: string }; + +CREATE DATASET GleambookUsersComposite(GleambookUserType) PRIMARY KEY gb,id; +CREATE DATASET GleambookMessagesComposite(GleambookMessageType) PRIMARY KEY gb,message_id; + +CREATE INDEX usrSinceIx ON GleambookUsersComposite(user_since: string); +CREATE INDEX authorIdIx ON GleambookMessagesComposite(author_id: string); \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.2.update.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.2.update.sqlpp new file mode 100644 index 0000000..43bd629 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.2.update.sqlpp @@ -0,0 +1,39 @@ +/* + * 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. + */ + +USE bigfun; + +INSERT INTO GleambookUsersComposite [ +{"gb": 1, "id": "1", "name": "name1", "user_since": '2010-07-22T00:00:00'}, +{"gb": 1, "id": "2", "name": "name2", "user_since": '2011-07-22T00:00:00'}, +{"gb": 2, "id": "3", "name": "name3", "user_since": '2010-09-22T00:00:00'}, +{"gb": 2, "id": "4", "name": "name4", "user_since": '2010-10-22T00:00:00'}, +{"gb": 3, "id": "5", "name": "name5", "user_since": '2013-07-22T00:00:00'} +]; + +INSERT INTO GleambookMessagesComposite [ +{"gb": 1, "message_id": "1", "author_id": "1", "message": "message1_1"}, +{"gb": 1, "message_id": "2", "author_id": "1", "message": "message2_1"}, +{"gb": 1, "message_id": "3", "author_id": "1", "message": "message3_1"}, +{"gb": 2, "message_id": "4", "author_id": "2", "message": "message1_2"}, +{"gb": 2, "message_id": "5", "author_id": "2", "message": "message2_2"}, +{"gb": 3, "message_id": "6", "author_id": "2", "message": "message3_2"}, +{"gb": 3, "message_id": "7", "author_id": "5", "message": "message1_5"}, +{"gb": 3, "message_id": "8", "author_id": "5", "message": "message2_5"} +]; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXSB-2700.3.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXSB-2700.3.query.sqlpp new file mode 100644 index 0000000..9d6dc06 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2700/query-ASTERIXSB-2700.3.query.sqlpp @@ -0,0 +1,30 @@ +/* + * 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. + */ + +USE bigfun; + +SET `compiler.sort.parallel` "false"; + +FROM (SELECT VALUE u + FROM GleambookUsersComposite u + WHERE u.user_since >= '2008-07-22T00:00:00' + ORDER BY u.id) AS user, GleambookMessagesComposite AS msg +WHERE msg.author_id /*+ indexnl */ = user.id +SELECT user.name AS uname, msg.message AS message +ORDER BY uname, message;; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.3.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.3.adm new file mode 100644 index 0000000..56bcafa --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2700/query-ASTERIXDB-2700.3.adm @@ -0,0 +1,8 @@ +{ "uname": "name1", "message": "message1_1" } +{ "uname": "name1", "message": "message2_1" } +{ "uname": "name1", "message": "message3_1" } +{ "uname": "name2", "message": "message1_2" } +{ "uname": "name2", "message": "message2_2" } +{ "uname": "name2", "message": "message3_2" } +{ "uname": "name5", "message": "message1_5" } +{ "uname": "name5", "message": "message2_5" } \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml index 6b5da4d..461e164 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml @@ -6190,6 +6190,11 @@ <output-dir compare="Text">insert_nulls_with_secondary_idx</output-dir> </compilation-unit> </test-case> + <test-case FilePath="misc"> + <compilation-unit name="query-ASTERIXDB-2700"> + <output-dir compare="Text">query-ASTERIXDB-2700</output-dir> + </compilation-unit> + </test-case> </test-group> <test-group name="index"> <test-group name="index/validations"> diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java index 9236545..706028b 100644 --- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java +++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java @@ -50,6 +50,7 @@ import org.apache.hyracks.algebricks.core.algebra.functions.IFunctionInfo; import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractLogicalOperator; import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractOperatorWithNestedPlans; import org.apache.hyracks.algebricks.core.algebra.operators.logical.AggregateOperator; +import org.apache.hyracks.algebricks.core.algebra.operators.logical.AssignOperator; import org.apache.hyracks.algebricks.core.algebra.operators.logical.DistinctOperator; import org.apache.hyracks.algebricks.core.algebra.operators.logical.ExchangeOperator; import org.apache.hyracks.algebricks.core.algebra.operators.logical.ForwardOperator; @@ -63,6 +64,7 @@ import org.apache.hyracks.algebricks.core.algebra.operators.physical.AbstractGro import org.apache.hyracks.algebricks.core.algebra.operators.physical.AbstractPreSortedDistinctByPOperator; import org.apache.hyracks.algebricks.core.algebra.operators.physical.AbstractStableSortPOperator; import org.apache.hyracks.algebricks.core.algebra.operators.physical.AggregatePOperator; +import org.apache.hyracks.algebricks.core.algebra.operators.physical.AssignPOperator; import org.apache.hyracks.algebricks.core.algebra.operators.physical.BroadcastExchangePOperator; import org.apache.hyracks.algebricks.core.algebra.operators.physical.HashPartitionExchangePOperator; import org.apache.hyracks.algebricks.core.algebra.operators.physical.HashPartitionMergeExchangePOperator; @@ -284,18 +286,18 @@ public class EnforceStructuralPropertiesRule implements IAlgebraicRewriteRule { printOp(op, context); } changed = true; - AbstractLogicalOperator nextOp = (AbstractLogicalOperator) op.getInputs().get(0).getValue(); - if (nextOp.getOperatorTag() == LogicalOperatorTag.PROJECT) { - nextOp = (AbstractLogicalOperator) nextOp.getInputs().get(0).getValue(); - } - opRef.setValue(nextOp); - // Now, transfer annotations from the original sort op. to this one. - AbstractLogicalOperator transferTo = nextOp; - if (transferTo.getOperatorTag() == LogicalOperatorTag.EXCHANGE) { - // remove duplicate exchange operator - transferTo = (AbstractLogicalOperator) transferTo.getInputs().get(0).getValue(); - } - transferTo.getAnnotations().putAll(op.getAnnotations()); + // replace the sort with empty assign (to handle cases where the sort might be sitting between exchanges) + // RemoveUnusedAssignAndAggregateRule should run after and decide whether to remove the assign or keep it + AssignOperator assignOperator = new AssignOperator(new ArrayList<>(0), new ArrayList<>(0)); + AssignPOperator assignPOperator = new AssignPOperator(); + assignOperator.setSourceLocation(opRef.getValue().getSourceLocation()); + assignOperator.setPhysicalOperator(assignPOperator); + assignOperator.getInputs().addAll(op.getInputs()); + opRef.setValue(assignOperator); + OperatorManipulationUtil.setOperatorMode(assignOperator); + OperatorPropertiesUtil.computeSchemaAndPropertiesRecIfNull(assignOperator, context); + context.computeAndSetTypeEnvironmentForOperator(assignOperator); + PhysicalOptimizationsUtil.computeFDsAndEquivalenceClasses(assignOperator, context); physOptimizeOp(opRef, required, nestedPlan, context); } return changed; diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java index eba3c91..a563f46 100644 --- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java +++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java @@ -90,7 +90,7 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule // we try to remove these operators if the produced variables from these // operators are not used. if (!assignedVarMap.isEmpty()) { - removeUnusedAssigns(opRef, context); + removeUnusedAssigns(opRef, false, null, context); } return isTransformed; @@ -139,8 +139,19 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule return assignVarsSetForThisOp; } - private void removeUnusedAssigns(Mutable<ILogicalOperator> opRef, IOptimizationContext context) - throws AlgebricksException { + /** + * Removes the assigned variables of an operator (left-hand side variables) if they are not used. It also removes + * the operator altogether when the operator is not assigning any more variables after removal of the variables + * (Except for few specific operators which cannot be removed such as UNIONALL). + * + * @param opRef the operator from which the assigned variables are to be removed. + * @param opInSubplan whether the operator is inside a subplan. + * @param parentOp the parent operator of {@code opRef} or null if it does not have one. + * @param context the optimization context. + * @throws AlgebricksException + */ + private void removeUnusedAssigns(Mutable<ILogicalOperator> opRef, boolean opInSubplan, ILogicalOperator parentOp, + IOptimizationContext context) throws AlgebricksException { AbstractLogicalOperator op = (AbstractLogicalOperator) opRef.getValue(); @@ -148,8 +159,7 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule while (removeFromAssigns(op, assignVarsSetForThisOp, context) == 0) { // UnionAllOperator cannot be removed since it has two branches. - if (op.getOperatorTag() == LogicalOperatorTag.AGGREGATE - || op.getOperatorTag() == LogicalOperatorTag.UNIONALL) { + if (!canRemoveOperator(op, opInSubplan, parentOp)) { break; } op = (AbstractLogicalOperator) op.getInputs().get(0).getValue(); @@ -161,7 +171,7 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule Iterator<Mutable<ILogicalOperator>> childIter = op.getInputs().iterator(); while (childIter.hasNext()) { Mutable<ILogicalOperator> cRef = childIter.next(); - removeUnusedAssigns(cRef, context); + removeUnusedAssigns(cRef, opInSubplan, op, context); } if (op.hasNestedPlans()) { @@ -170,7 +180,7 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule while (planIter.hasNext()) { ILogicalPlan p = planIter.next(); for (Mutable<ILogicalOperator> r : p.getRoots()) { - removeUnusedAssigns(r, context); + removeUnusedAssigns(r, true, null, context); } } @@ -420,6 +430,22 @@ public class RemoveUnusedAssignAndAggregateRule implements IAlgebraicRewriteRule } } + private static boolean canRemoveOperator(ILogicalOperator op, boolean opInsideSubplan, ILogicalOperator parentOp) { + LogicalOperatorTag opTag = op.getOperatorTag(); + if (opTag == LogicalOperatorTag.AGGREGATE || opTag == LogicalOperatorTag.UNIONALL) { + return false; + } + if (!opInsideSubplan) { + // for an operator in the outer plan, do not remove if it's sitting between exchanges or it's root+exchange + boolean childIsExchange = + op.hasInputs() && op.getInputs().get(0).getValue().getOperatorTag() == LogicalOperatorTag.EXCHANGE; + if (childIsExchange && (parentOp == null || parentOp.getOperatorTag() == LogicalOperatorTag.EXCHANGE)) { + return false; + } + } + return true; + } + private void clear() { assignedVarMap.clear(); assignedVarSet.clear();
