[ASTERIXDB-2886][COMP] Fix RemoveRedundantVariablesRule
- user model changes: no
- storage format changes: no
- interface changes: no
Details:
- Change RemoveRedundantVariablesRule to
operate on the whole plan at once instead
of working incrementally on each operator
Change-Id: Ia6cec065614ad5e2b14e7b160cf3c9de7215618a
Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/11103
Integration-Tests: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Tested-by: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reviewed-by: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Reviewed-by: Ali Alsuliman <ali.al.solaiman@gmail.com>
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.1.ddl.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.1.ddl.sqlpp
new file mode 100644
index 0000000..ab6b723
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.1.ddl.sqlpp
@@ -0,0 +1,43 @@
+/*
+ * 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 test1 if exists;
+create dataverse test1;
+use test1;
+
+create dataset a(
+ c_a1 bigint not unknown,
+ c_a2 bigint,
+ c_a3 string
+) primary key c_a1;
+
+create index ia2 on a(c_a2);
+
+create index ia3 on a(c_a3);
+
+create dataset b(
+ c_b1 bigint not unknown,
+ c_b2 bigint,
+ c_b3 string
+) primary key c_b1;
+
+create index ib2 on b(c_b2);
+
+create index ib3 on b(c_b3);
+
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.2.ddl.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.2.ddl.sqlpp
new file mode 100644
index 0000000..621d057
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.2.ddl.sqlpp
@@ -0,0 +1,43 @@
+/*
+ * 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 test2 if exists;
+create dataverse test2;
+use test2;
+
+create dataset a(
+ c_a1 bigint not unknown,
+ c_a2 bigint,
+ c_a3 string
+) primary key c_a1;
+
+create index ia2 on a(c_a2);
+
+create index ia3 on a(c_a3);
+
+create dataset b(
+ c_b1 bigint not unknown,
+ c_b2 bigint,
+ c_b3 string
+) primary key c_b1;
+
+create index ib2 on b(c_b2);
+
+create index ib3 on b(c_b3);
+
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.3.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.3.query.sqlpp
new file mode 100644
index 0000000..e48a1e1
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.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.
+ */
+
+select ds.DataverseName, ds.DatasetName, dt.Derived.IsAnonymous, indexes
+from Metadata.`Dataset` as ds left join Metadata.`Datatype` dt
+on ds.DataverseName = dt.DataverseName and ds.DatatypeName = dt.DatatypeName
+let indexes = (
+ select idx.IndexName
+ from Metadata.`Index` as idx
+ where idx.DataverseName = ds.DataverseName and idx.DatasetName = ds.DatasetName
+ order by idx.IndexName
+)
+where ds.DataverseName like "test%"
+order by ds.DataverseName, ds.DatasetName;
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan-select/push-limit-to-primary-scan-select.11.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan-select/push-limit-to-primary-scan-select.11.adm
index 85cf5c5..28c74ac 100644
--- a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan-select/push-limit-to-primary-scan-select.11.adm
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan-select/push-limit-to-primary-scan-select.11.adm
@@ -2,9 +2,9 @@
-- DISTRIBUTE_RESULT |LOCAL|
exchange
-- ONE_TO_ONE_EXCHANGE |LOCAL|
- aggregate [$$202] <- [agg-sql-sum($$235)]
+ aggregate [$$202] <- [agg-sql-sum($$231)]
-- AGGREGATE |LOCAL|
- aggregate [$$235] <- [agg-sql-count(1)]
+ aggregate [$$231] <- [agg-sql-count(1)]
-- AGGREGATE |LOCAL|
exchange
-- ONE_TO_ONE_EXCHANGE |UNPARTITIONED|
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan/push-limit-to-primary-scan.7.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan/push-limit-to-primary-scan.7.adm
index f830e9b..f8a800a 100644
--- a/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan/push-limit-to-primary-scan.7.adm
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/limit/push-limit-to-primary-scan/push-limit-to-primary-scan.7.adm
@@ -2,9 +2,9 @@
-- DISTRIBUTE_RESULT |LOCAL|
exchange
-- ONE_TO_ONE_EXCHANGE |LOCAL|
- aggregate [$$180] <- [agg-sql-sum($$209)]
+ aggregate [$$180] <- [agg-sql-sum($$205)]
-- AGGREGATE |LOCAL|
- aggregate [$$209] <- [agg-sql-count(1)]
+ aggregate [$$205] <- [agg-sql-count(1)]
-- AGGREGATE |LOCAL|
exchange
-- ONE_TO_ONE_EXCHANGE |UNPARTITIONED|
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.3.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.3.adm
new file mode 100644
index 0000000..7065d25
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/misc/query-ASTERIXDB-2886/query-ASTERIXDB-2886.3.adm
@@ -0,0 +1,4 @@
+{ "DataverseName": "test1", "DatasetName": "a", "IsAnonymous": true, "indexes": [ { "IndexName": "a" }, { "IndexName": "ia2" }, { "IndexName": "ia3" } ] }
+{ "DataverseName": "test1", "DatasetName": "b", "IsAnonymous": true, "indexes": [ { "IndexName": "b" }, { "IndexName": "ib2" }, { "IndexName": "ib3" } ] }
+{ "DataverseName": "test2", "DatasetName": "a", "IsAnonymous": true, "indexes": [ { "IndexName": "a" }, { "IndexName": "ia2" }, { "IndexName": "ia3" } ] }
+{ "DataverseName": "test2", "DatasetName": "b", "IsAnonymous": true, "indexes": [ { "IndexName": "b" }, { "IndexName": "ib2" }, { "IndexName": "ib3" } ] }
\ No newline at end of file
diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/union/union_opt_1/union_opt_1.11.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/union/union_opt_1/union_opt_1.11.adm
index 1b39645..a809a8e 100644
--- a/asterixdb/asterix-app/src/test/resources/runtimets/results/union/union_opt_1/union_opt_1.11.adm
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/union/union_opt_1/union_opt_1.11.adm
@@ -6,7 +6,7 @@
-- STREAM_LIMIT |UNPARTITIONED|
exchange
-- RANDOM_MERGE_EXCHANGE |PARTITIONED|
- union ($$151, $$310, $$t)
+ union ($$151, $$178, $$t)
-- UNION_ALL |PARTITIONED|
exchange
-- ONE_TO_ONE_EXCHANGE |PARTITIONED|
@@ -58,7 +58,7 @@
-- EMPTY_TUPLE_SOURCE |PARTITIONED|
exchange
-- ONE_TO_ONE_EXCHANGE |PARTITIONED|
- union ($$345, $$356, $$310)
+ union ($$345, $$354, $$178)
-- UNION_ALL |PARTITIONED|
exchange
-- RANDOM_PARTITION_EXCHANGE |PARTITIONED|
@@ -84,9 +84,9 @@
-- EMPTY_TUPLE_SOURCE |PARTITIONED|
exchange
-- RANDOM_PARTITION_EXCHANGE |PARTITIONED|
- project ([$$356])
+ project ([$$354])
-- STREAM_PROJECT |PARTITIONED|
- assign [$$356] <- [{"two": $$186}]
+ assign [$$354] <- [{"two": $$186}]
-- ASSIGN |PARTITIONED|
limit 4
-- STREAM_LIMIT |PARTITIONED|
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 1f955e6..c9e37e4 100644
--- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml
+++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml
@@ -6841,6 +6841,11 @@
</compilation-unit>
</test-case>
<test-case FilePath="misc">
+ <compilation-unit name="query-ASTERIXDB-2886">
+ <output-dir compare="Text">query-ASTERIXDB-2886</output-dir>
+ </compilation-unit>
+ </test-case>
+ <test-case FilePath="misc">
<compilation-unit name="unsupported_parameter">
<output-dir compare="Text">none</output-dir>
<expected-error>Query parameter compiler.joinmem is not supported</expected-error>
diff --git a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java
index 5386193..3760be5 100644
--- a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java
+++ b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java
@@ -36,7 +36,6 @@
import org.apache.hyracks.algebricks.core.algebra.base.LogicalOperatorTag;
import org.apache.hyracks.algebricks.core.algebra.base.LogicalVariable;
import org.apache.hyracks.algebricks.core.algebra.expressions.AbstractFunctionCallExpression;
-import org.apache.hyracks.algebricks.core.algebra.expressions.AbstractLogicalExpression;
import org.apache.hyracks.algebricks.core.algebra.expressions.VariableReferenceExpression;
import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractLogicalOperator;
import org.apache.hyracks.algebricks.core.algebra.operators.logical.AbstractOperatorWithNestedPlans;
@@ -69,26 +68,33 @@
public class RemoveRedundantVariablesRule implements IAlgebraicRewriteRule {
private final VariableSubstitutionVisitor substVisitor = new VariableSubstitutionVisitor();
- private final Map<LogicalVariable, List<LogicalVariable>> equivalentVarsMap =
- new HashMap<LogicalVariable, List<LogicalVariable>>();
+
+ private final Map<LogicalVariable, List<LogicalVariable>> equivalentVarsMap = new HashMap<>();
@Override
public boolean rewritePost(Mutable<ILogicalOperator> opRef, IOptimizationContext context)
throws AlgebricksException {
+ return false;
+ }
+
+ @Override
+ public boolean rewritePre(Mutable<ILogicalOperator> opRef, IOptimizationContext context)
+ throws AlgebricksException {
if (context.checkIfInDontApplySet(this, opRef.getValue())) {
return false;
}
- boolean modified = removeRedundantVariables(opRef, context);
- if (modified) {
- context.computeAndSetTypeEnvironmentForOperator(opRef.getValue());
- }
- return modified;
+ clear();
+ return removeRedundantVariables(opRef, true, context);
+ }
+
+ private void clear() {
+ equivalentVarsMap.clear();
}
private void updateEquivalenceClassMap(LogicalVariable lhs, LogicalVariable rhs) {
List<LogicalVariable> equivalentVars = equivalentVarsMap.get(rhs);
if (equivalentVars == null) {
- equivalentVars = new ArrayList<LogicalVariable>();
+ equivalentVars = new ArrayList<>();
// The first element in the list is the bottom-most representative which will replace all equivalent vars.
equivalentVars.add(rhs);
equivalentVars.add(lhs);
@@ -97,12 +103,32 @@
equivalentVarsMap.put(lhs, equivalentVars);
}
- private boolean removeRedundantVariables(Mutable<ILogicalOperator> opRef, IOptimizationContext context)
- throws AlgebricksException {
+ private LogicalVariable findEquivalentRepresentativeVar(LogicalVariable var) {
+ List<LogicalVariable> equivalentVars = equivalentVarsMap.get(var);
+ if (equivalentVars == null) {
+ return null;
+ }
+ LogicalVariable representativeVar = equivalentVars.get(0);
+ return var.equals(representativeVar) ? null : representativeVar;
+ }
+
+ private boolean removeRedundantVariables(Mutable<ILogicalOperator> opRef, boolean first,
+ IOptimizationContext context) throws AlgebricksException {
AbstractLogicalOperator op = (AbstractLogicalOperator) opRef.getValue();
+ if (!first) {
+ context.addToDontApplySet(this, op);
+ }
+
LogicalOperatorTag opTag = op.getOperatorTag();
boolean modified = false;
+ // Recurse into children.
+ for (Mutable<ILogicalOperator> inputOpRef : op.getInputs()) {
+ if (removeRedundantVariables(inputOpRef, false, context)) {
+ modified = true;
+ }
+ }
+
// Update equivalence class map.
if (opTag == LogicalOperatorTag.ASSIGN) {
AssignOperator assignOp = (AssignOperator) op;
@@ -142,7 +168,7 @@
AbstractOperatorWithNestedPlans opWithNestedPlan = (AbstractOperatorWithNestedPlans) op;
for (ILogicalPlan nestedPlan : opWithNestedPlan.getNestedPlans()) {
for (Mutable<ILogicalOperator> rootRef : nestedPlan.getRoots()) {
- if (removeRedundantVariables(rootRef, context)) {
+ if (removeRedundantVariables(rootRef, false, context)) {
modified = true;
}
}
@@ -158,14 +184,8 @@
if (modified) {
context.computeAndSetTypeEnvironmentForOperator(op);
- context.addToDontApplySet(this, op);
}
- // Clears the equivalent variable map if the current operator is the root operator
- // in the query plan.
- if (opTag == LogicalOperatorTag.DISTRIBUTE_RESULT || opTag == LogicalOperatorTag.SINK) {
- equivalentVarsMap.clear();
- }
return modified;
}
@@ -227,38 +247,34 @@
* We cannot use the VariableSubstitutionVisitor here because the project ops
* maintain their variables as a list and not as expressions.
*/
- private boolean replaceProjectVars(ProjectOperator op) throws AlgebricksException {
+ private boolean replaceProjectVars(ProjectOperator op) {
List<LogicalVariable> vars = op.getVariables();
int size = vars.size();
boolean modified = false;
for (int i = 0; i < size; i++) {
LogicalVariable var = vars.get(i);
- List<LogicalVariable> equivalentVars = equivalentVarsMap.get(var);
- if (equivalentVars == null) {
- continue;
- }
- // Replace with equivalence class representative.
- LogicalVariable representative = equivalentVars.get(0);
- if (representative != var) {
- vars.set(i, equivalentVars.get(0));
+ LogicalVariable representativeVar = findEquivalentRepresentativeVar(var);
+ if (representativeVar != null) {
+ // Replace with equivalence class representative.
+ vars.set(i, representativeVar);
modified = true;
}
}
return modified;
}
- private boolean replaceUnionAllVars(UnionAllOperator op) throws AlgebricksException {
+ private boolean replaceUnionAllVars(UnionAllOperator op) {
boolean modified = false;
for (Triple<LogicalVariable, LogicalVariable, LogicalVariable> varMapping : op.getVariableMappings()) {
- List<LogicalVariable> firstEquivalentVars = equivalentVarsMap.get(varMapping.first);
- List<LogicalVariable> secondEquivalentVars = equivalentVarsMap.get(varMapping.second);
// Replace variables with their representative.
- if (firstEquivalentVars != null) {
- varMapping.first = firstEquivalentVars.get(0);
+ LogicalVariable firstRepresentativeVar = findEquivalentRepresentativeVar(varMapping.first);
+ if (firstRepresentativeVar != null) {
+ varMapping.first = firstRepresentativeVar;
modified = true;
}
- if (secondEquivalentVars != null) {
- varMapping.second = secondEquivalentVars.get(0);
+ LogicalVariable secondRepresentativeVar = findEquivalentRepresentativeVar(varMapping.second);
+ if (secondRepresentativeVar != null) {
+ varMapping.second = secondRepresentativeVar;
modified = true;
}
}
@@ -269,17 +285,13 @@
@Override
public boolean transform(Mutable<ILogicalExpression> exprRef) {
ILogicalExpression e = exprRef.getValue();
- switch (((AbstractLogicalExpression) e).getExpressionTag()) {
+ switch (e.getExpressionTag()) {
case VARIABLE: {
// Replace variable references with their equivalent representative in the equivalence class map.
VariableReferenceExpression varRefExpr = (VariableReferenceExpression) e;
LogicalVariable var = varRefExpr.getVariableReference();
- List<LogicalVariable> equivalentVars = equivalentVarsMap.get(var);
- if (equivalentVars == null) {
- return false;
- }
- LogicalVariable representative = equivalentVars.get(0);
- if (representative != var) {
+ LogicalVariable representative = findEquivalentRepresentativeVar(var);
+ if (representative != null) {
varRefExpr.setVariable(representative);
return true;
}