You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@asterixdb.apache.org by AsterixDB Code Review <do...@asterix-gerrit.ics.uci.edu> on 2022/01/31 22:43:53 UTC

Change in asterixdb[neo]: [ASTERIXDB-3007][COMP] Fix ConsolidateWindowOperatorsRule

From Dmitry Lychagin <dm...@couchbase.com>:

Dmitry Lychagin has uploaded this change for review. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15063 )


Change subject: [ASTERIXDB-3007][COMP] Fix ConsolidateWindowOperatorsRule
......................................................................

[ASTERIXDB-3007][COMP] Fix ConsolidateWindowOperatorsRule

- user model changes: no
- storage format changes: no
- interface changes: no

Details:
- Fix ConsolidateWindowOperatorsRule to correclty merge window
  operator with subplans into window operator without subplans
- Fix deep copy visitors for window operator with subplans
- Add compiler sanity check code to verify that each nested tuple
  source operator correctly points to its datasource operator

Change-Id: Ib9077a0331ab57cdd449426be77f05741d0778cc
---
A asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/win_opt_02/win_opt_02_1.sqlpp
A asterixdb/asterix-app/src/test/resources/optimizerts/results/window/win_opt_02/win_opt_02_1.plan
A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/win_opt_02/win_opt_02.10.query.sqlpp
A asterixdb/asterix-app/src/test/resources/runtimets/results/window/win_opt_02/win_opt_02.10.adm
M hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/LogicalOperatorDeepCopyWithNewVariablesVisitor.java
M hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/OperatorDeepCopyVisitor.java
M hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java
M hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateWindowOperatorsRule.java
8 files changed, 159 insertions(+), 7 deletions(-)



  git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/63/15063/1

diff --git a/asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/win_opt_02/win_opt_02_1.sqlpp b/asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/win_opt_02/win_opt_02_1.sqlpp
new file mode 100644
index 0000000..def3a02
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/win_opt_02/win_opt_02_1.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  : Test fix for ASTERIXDB-3007
+ * Expected Res : SUCCESS
+ */
+
+with ds1 as (
+  select r as t, r*r as x
+  from range(1, 10) r
+)
+
+select t, x, dt, dx, int(v) as v, int(a) as a
+from ds1
+let dt = t - lag(t) over (order by t),
+    dx = x - lag(x) over (order by t),
+    v = dx/dt,
+    a = v - lag(v) over (order by t)
+order by t;
\ No newline at end of file
diff --git a/asterixdb/asterix-app/src/test/resources/optimizerts/results/window/win_opt_02/win_opt_02_1.plan b/asterixdb/asterix-app/src/test/resources/optimizerts/results/window/win_opt_02/win_opt_02_1.plan
new file mode 100644
index 0000000..931e417
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/optimizerts/results/window/win_opt_02/win_opt_02_1.plan
@@ -0,0 +1,23 @@
+-- DISTRIBUTE_RESULT  |LOCAL|
+  -- ONE_TO_ONE_EXCHANGE  |LOCAL|
+    -- STREAM_PROJECT  |LOCAL|
+      -- ASSIGN  |LOCAL|
+        -- STREAM_PROJECT  |LOCAL|
+          -- ASSIGN  |LOCAL|
+            -- STREAM_PROJECT  |LOCAL|
+              -- WINDOW  |LOCAL|
+                      {
+                        -- AGGREGATE  |LOCAL|
+                          -- NESTED_TUPLE_SOURCE  |LOCAL|
+                      }
+                -- WINDOW  |LOCAL|
+                        {
+                          -- AGGREGATE  |LOCAL|
+                            -- NESTED_TUPLE_SOURCE  |LOCAL|
+                        }
+                  -- WINDOW_STREAM  |LOCAL|
+                    -- ONE_TO_ONE_EXCHANGE  |LOCAL|
+                      -- STABLE_SORT [$$r(ASC)]  |LOCAL|
+                        -- ONE_TO_ONE_EXCHANGE  |UNPARTITIONED|
+                          -- UNNEST  |UNPARTITIONED|
+                            -- EMPTY_TUPLE_SOURCE  |UNPARTITIONED|
\ No newline at end of file
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/win_opt_02/win_opt_02.10.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/win_opt_02/win_opt_02.10.query.sqlpp
new file mode 100644
index 0000000..a6f448c
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/win_opt_02/win_opt_02.10.query.sqlpp
@@ -0,0 +1,37 @@
+/*
+ * 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  : Test fix for ASTERIXDB-3007
+ * Expected Res : SUCCESS
+ */
+
+use test;
+
+with ds1 as (
+  select r as t, r*r as x
+  from range(1, 10) r
+)
+
+select t, x, dt, dx, int(v) as v, int(a) as a
+from ds1
+let dt = t - lag(t) over (order by t),
+    dx = x - lag(x) over (order by t),
+    v = dx/dt,
+    a = v - lag(v) over (order by t)
+order by t;
\ No newline at end of file
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/window/win_opt_02/win_opt_02.10.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/window/win_opt_02/win_opt_02.10.adm
new file mode 100644
index 0000000..29322df
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/window/win_opt_02/win_opt_02.10.adm
@@ -0,0 +1,10 @@
+{ "t": 1, "x": 1, "dt": null, "dx": null, "v": null, "a": null }
+{ "t": 2, "x": 4, "dt": 1, "dx": 3, "v": 3, "a": null }
+{ "t": 3, "x": 9, "dt": 1, "dx": 5, "v": 5, "a": 2 }
+{ "t": 4, "x": 16, "dt": 1, "dx": 7, "v": 7, "a": 2 }
+{ "t": 5, "x": 25, "dt": 1, "dx": 9, "v": 9, "a": 2 }
+{ "t": 6, "x": 36, "dt": 1, "dx": 11, "v": 11, "a": 2 }
+{ "t": 7, "x": 49, "dt": 1, "dx": 13, "v": 13, "a": 2 }
+{ "t": 8, "x": 64, "dt": 1, "dx": 15, "v": 15, "a": 2 }
+{ "t": 9, "x": 81, "dt": 1, "dx": 17, "v": 17, "a": 2 }
+{ "t": 10, "x": 100, "dt": 1, "dx": 19, "v": 19, "a": 2 }
\ No newline at end of file
diff --git a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/LogicalOperatorDeepCopyWithNewVariablesVisitor.java b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/LogicalOperatorDeepCopyWithNewVariablesVisitor.java
index 199c2e1..e242531 100644
--- a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/LogicalOperatorDeepCopyWithNewVariablesVisitor.java
+++ b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/LogicalOperatorDeepCopyWithNewVariablesVisitor.java
@@ -619,11 +619,11 @@
         List<LogicalVariable> varCopy = deepCopyVariableList(op.getVariables());
         List<Mutable<ILogicalExpression>> exprCopy =
                 exprDeepCopyVisitor.deepCopyExpressionReferenceList(op.getExpressions());
-        List<ILogicalPlan> nestedPlansCopy = new ArrayList<>();
         WindowOperator opCopy = new WindowOperator(partitionExprCopy, orderExprCopy, frameValueExprCopy,
                 frameStartExprCopy, frameStartValidationExprCopy, frameEndExprCopy, frameEndValidationExprCopy,
                 frameExcludeExprCopy, op.getFrameExcludeNegationStartIdx(), frameExcludeUnaryExprCopy,
-                frameOffsetExprCopy, op.getFrameMaxObjects(), varCopy, exprCopy, nestedPlansCopy);
+                frameOffsetExprCopy, op.getFrameMaxObjects(), varCopy, exprCopy, null);
+        List<ILogicalPlan> nestedPlansCopy = opCopy.getNestedPlans();
         deepCopyInputsAnnotationsAndExecutionMode(op, arg, opCopy);
         deepCopyPlanList(op.getNestedPlans(), nestedPlansCopy, opCopy);
         return opCopy;
diff --git a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/OperatorDeepCopyVisitor.java b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/OperatorDeepCopyVisitor.java
index 7b67af1..c2ee661 100644
--- a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/OperatorDeepCopyVisitor.java
+++ b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/OperatorDeepCopyVisitor.java
@@ -448,11 +448,11 @@
         deepCopyVars(newVariables, op.getVariables());
         List<Mutable<ILogicalExpression>> newExpressions = new ArrayList<>();
         deepCopyExpressionRefs(newExpressions, op.getExpressions());
-        List<ILogicalPlan> newNestedPlans = new ArrayList<>();
         WindowOperator newWinOp = new WindowOperator(newPartitionExprs, newOrderExprs, newFrameValueExprs,
                 newFrameStartExprs, newFrameStartValidationExprs, newFrameEndExprs, newFrameEndValidationExprs,
                 newFrameExclusionExprs, op.getFrameExcludeNegationStartIdx(), newFrameExcludeUnaryExpr,
-                newFrameOffsetExpr, op.getFrameMaxObjects(), newVariables, newExpressions, newNestedPlans);
+                newFrameOffsetExpr, op.getFrameMaxObjects(), newVariables, newExpressions, null);
+        List<ILogicalPlan> newNestedPlans = newWinOp.getNestedPlans();
         for (ILogicalPlan nestedPlan : op.getNestedPlans()) {
             newNestedPlans.add(OperatorManipulationUtil.deepCopy(nestedPlan, newWinOp));
         }
diff --git a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java
index a072d11..fed1c75 100644
--- a/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java
+++ b/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java
@@ -46,9 +46,11 @@
 import org.apache.hyracks.algebricks.core.algebra.expressions.AbstractFunctionCallExpression;
 import org.apache.hyracks.algebricks.core.algebra.expressions.IVariableTypeEnvironment;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractOperatorWithNestedPlans;
+import org.apache.hyracks.algebricks.core.algebra.operators.logical.NestedTupleSourceOperator;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.visitors.VariableUtilities;
 import org.apache.hyracks.algebricks.core.algebra.prettyprint.IPlanPrettyPrinter;
 import org.apache.hyracks.algebricks.core.algebra.typing.ITypingContext;
+import org.apache.hyracks.algebricks.core.algebra.util.OperatorManipulationUtil;
 import org.apache.hyracks.algebricks.core.algebra.util.OperatorPropertiesUtil;
 import org.apache.hyracks.algebricks.core.algebra.visitors.ILogicalExpressionReferenceTransform;
 
@@ -76,6 +78,11 @@
 
     private static final String ERROR_MESSAGE_TEMPLATE_6 = "undefined used variables %s in %s";
 
+    private static final String ERROR_MESSAGE_TEMPLATE_7 =
+            "unexpected source operator in NestedTupleSourceOperator: %s. Expected source operator %s";
+
+    private static final String ERROR_MESSAGE_TEMPLATE_8 = "unexpected leaf operator in nested plan: %s";
+
     public static final Comparator<LogicalVariable> VARIABLE_CMP = Comparator.comparing(LogicalVariable::toString);
 
     private final ExpressionReferenceVerifierVisitor exprVisitor = new ExpressionReferenceVerifierVisitor();
@@ -185,7 +192,10 @@
         if (op instanceof AbstractOperatorWithNestedPlans) {
             children = new ArrayList<>(children);
             for (ILogicalPlan nestedPlan : ((AbstractOperatorWithNestedPlans) op).getNestedPlans()) {
-                children.addAll(nestedPlan.getRoots());
+                for (Mutable<ILogicalOperator> nestedRootRef : nestedPlan.getRoots()) {
+                    checkLeafOperatorsInNestedPlan(op, nestedRootRef);
+                    children.add(nestedRootRef);
+                }
             }
         }
         return children;
@@ -262,6 +272,28 @@
         }
     }
 
+    private void checkLeafOperatorsInNestedPlan(ILogicalOperator op, Mutable<ILogicalOperator> rootRef) throws AlgebricksException {
+        for (Mutable<ILogicalOperator> leafRef : OperatorManipulationUtil.findLeafDescendantsOrSelf(rootRef)) {
+            ILogicalOperator leafOp = leafRef.getValue();
+            switch (leafOp.getOperatorTag()) {
+                case EMPTYTUPLESOURCE:
+                    break;
+                case NESTEDTUPLESOURCE:
+                    NestedTupleSourceOperator ntsOp = (NestedTupleSourceOperator) leafOp;
+                    ILogicalOperator ntsSrcOp = ntsOp.getDataSourceReference().getValue();
+                    if (ntsSrcOp != op) {
+                        throw new AlgebricksException(String.format(ERROR_MESSAGE_TEMPLATE_7,
+                                PlanStabilityVerifier.printOperator(ntsSrcOp, prettyPrinter),
+                                PlanStabilityVerifier.printOperator(op, prettyPrinter)));
+                    }
+                    break;
+                default:
+                    throw new AlgebricksException(String.format(ERROR_MESSAGE_TEMPLATE_8,
+                            PlanStabilityVerifier.printOperator(leafOp, prettyPrinter)));
+            }
+        }
+    }
+
     private void raiseException(String sharedReferenceKind, String sharedEntity, ILogicalOperator firstOp,
             ILogicalOperator secondOp) throws AlgebricksException {
         String errorMessage;
diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateWindowOperatorsRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateWindowOperatorsRule.java
index 2ec0654..a226c36 100644
--- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateWindowOperatorsRule.java
+++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateWindowOperatorsRule.java
@@ -33,10 +33,12 @@
 import org.apache.hyracks.algebricks.core.algebra.base.LogicalVariable;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractLogicalOperator;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.AggregateOperator;
+import org.apache.hyracks.algebricks.core.algebra.operators.logical.NestedTupleSourceOperator;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.WindowOperator;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.visitors.IsomorphismOperatorVisitor;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.visitors.IsomorphismUtilities;
 import org.apache.hyracks.algebricks.core.algebra.operators.logical.visitors.VariableUtilities;
+import org.apache.hyracks.algebricks.core.algebra.util.OperatorManipulationUtil;
 import org.apache.hyracks.algebricks.core.algebra.util.OperatorPropertiesUtil;
 import org.apache.hyracks.algebricks.core.rewriter.base.IAlgebraicRewriteRule;
 
@@ -82,7 +84,9 @@
 
         Set<LogicalVariable> used1 = new HashSet<>();
         VariableUtilities.getUsedVariables(winOp1, used1);
-        if (!OperatorPropertiesUtil.disjoint(winOp2.getVariables(), used1)) {
+        Set<LogicalVariable> produced2 = new HashSet<>();
+        VariableUtilities.getProducedVariables(winOp2, produced2);
+        if (!OperatorPropertiesUtil.disjoint(produced2, used1)) {
             return false;
         }
 
@@ -130,7 +134,6 @@
             aggTo.getExpressions().addAll(aggFrom.getExpressions());
             context.computeAndSetTypeEnvironmentForOperator(aggTo);
         } else {
-            setAll(winOpTo.getNestedPlans(), winOpFrom.getNestedPlans());
             setAll(winOpTo.getFrameValueExpressions(), winOpFrom.getFrameValueExpressions());
             setAll(winOpTo.getFrameStartExpressions(), winOpFrom.getFrameStartExpressions());
             setAll(winOpTo.getFrameStartValidationExpressions(), winOpFrom.getFrameStartValidationExpressions());
@@ -141,6 +144,18 @@
             winOpTo.getFrameExcludeUnaryExpression().setValue(winOpFrom.getFrameExcludeUnaryExpression().getValue());
             winOpTo.getFrameOffsetExpression().setValue(winOpFrom.getFrameOffsetExpression().getValue());
             winOpTo.setFrameMaxObjects(winOpFrom.getFrameMaxObjects());
+            // move nested plans
+            for (ILogicalPlan fromNestedPlan : winOpFrom.getNestedPlans()) {
+                for (Mutable<ILogicalOperator> rootRef : fromNestedPlan.getRoots()) {
+                    for (Mutable<ILogicalOperator> leafRef : OperatorManipulationUtil.findLeafDescendantsOrSelf(rootRef)) {
+                        ILogicalOperator leafOp = leafRef.getValue();
+                        if (leafOp.getOperatorTag() == LogicalOperatorTag.NESTEDTUPLESOURCE) {
+                            ((NestedTupleSourceOperator)leafOp).getDataSourceReference().setValue(winOpTo);
+                        }
+                    }
+                }
+                winOpTo.getNestedPlans().add(fromNestedPlan);
+            }
         }
         return true;
     }

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15063
To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-Project: asterixdb
Gerrit-Branch: neo
Gerrit-Change-Id: Ib9077a0331ab57cdd449426be77f05741d0778cc
Gerrit-Change-Number: 15063
Gerrit-PatchSet: 1
Gerrit-Owner: Dmitry Lychagin <dm...@couchbase.com>
Gerrit-MessageType: newchange