>From Wail Alkowaileet <[email protected]>:
Wail Alkowaileet has uploaded this change for review. (
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17589 )
Change subject: [ASTERIXDB-3205][COMP] Avoid inlining non-pure functions in
aggregate functions
......................................................................
[ASTERIXDB-3205][COMP] Avoid inlining non-pure functions in aggregate functions
- user model changes: no
- storage format changes: no
- interface changes: no
Details:
Currently, the compiler blindly inlines all functions
from assign operators into aggregate functions. However,
non-pure functions should not be inlined.
Change-Id: Ib3fbd684f1732a276d6033bf38507df41d33b843
---
M
hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java
1 file changed, 41 insertions(+), 16 deletions(-)
git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb
refs/changes/89/17589/1
diff --git
a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java
b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java
index d966ed1..5e77507 100644
---
a/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java
+++
b/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/InlineAssignIntoAggregateRule.java
@@ -84,11 +84,15 @@
}
AssignOperator assignOp = (AssignOperator) op2;
VarExprSubstitution ves = new
VarExprSubstitution(assignOp.getVariables(), assignOp.getExpressions());
- inlineVariables(aggOp, ves);
- List<Mutable<ILogicalOperator>> op1InpList = aggOp.getInputs();
- op1InpList.clear();
- op1InpList.add(op2.getInputs().get(0));
- return true;
+ return inlineVariables(aggOp, ves);
+ /*
+ * TODO This is dangerous! Currently, we do not consolidate assigns in
a subplan. Hence, this does not impose
+ * any issue at this time. But in the future, we need to be
careful about removing the assign operator.
+ * TODO Why not to rely on RemoveUnusedAssignAndAggregateRule?
+ */
+ // List<Mutable<ILogicalOperator>> op1InpList =
aggOp.getInputs();
+ // op1InpList.clear();
+ // op1InpList.add(op2.getInputs().get(0));
}
private boolean inlineOuterInputAssignIntoAgg(AggregateOperator aggOp,
@@ -107,18 +111,20 @@
for (Mutable<ILogicalExpression> exprRef : aggOp.getExpressions()) {
ILogicalExpression expr = exprRef.getValue();
Pair<Boolean, ILogicalExpression> p = expr.accept(ves, null);
- if (p.first) {
- exprRef.setValue(p.second);
+ ILogicalExpression originalExpr = p.second;
+ if (p.first & originalExpr.isFunctional()) {
+ exprRef.setValue(originalExpr);
inlined = true;
}
}
return inlined;
}
- private class VarExprSubstitution extends
AbstractConstVarFunVisitor<Pair<Boolean, ILogicalExpression>, Void> {
+ private static class VarExprSubstitution
+ extends AbstractConstVarFunVisitor<Pair<Boolean,
ILogicalExpression>, Void> {
- private List<LogicalVariable> variables;
- private List<Mutable<ILogicalExpression>> expressions;
+ private final List<LogicalVariable> variables;
+ private final List<Mutable<ILogicalExpression>> expressions;
public VarExprSubstitution(List<LogicalVariable> variables,
List<Mutable<ILogicalExpression>> expressions) {
this.variables = variables;
@@ -127,7 +133,7 @@
@Override
public Pair<Boolean, ILogicalExpression>
visitConstantExpression(ConstantExpression expr, Void arg) {
- return new Pair<Boolean, ILogicalExpression>(false, expr);
+ return new Pair<>(false, expr);
}
@Override
@@ -137,12 +143,13 @@
for (Mutable<ILogicalExpression> eRef : expr.getArguments()) {
ILogicalExpression e = eRef.getValue();
Pair<Boolean, ILogicalExpression> p = e.accept(this, arg);
- if (p.first) {
- eRef.setValue(p.second.cloneExpression());
+ ILogicalExpression originalExpr = p.second;
+ if (p.first & originalExpr.isFunctional()) {
+ eRef.setValue(originalExpr.cloneExpression());
changed = true;
}
}
- return new Pair<Boolean, ILogicalExpression>(changed, expr);
+ return new Pair<>(changed, expr);
}
@Override
@@ -151,9 +158,9 @@
LogicalVariable v = expr.getVariableReference();
int idx = variables.indexOf(v);
if (idx < 0) {
- return new Pair<Boolean, ILogicalExpression>(false, expr);
+ return new Pair<>(false, expr);
} else {
- return new Pair<Boolean, ILogicalExpression>(true,
expressions.get(idx).getValue());
+ return new Pair<>(true, expressions.get(idx).getValue());
}
}
--
To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17589
To unsubscribe, or for help writing mail filters, visit
https://asterix-gerrit.ics.uci.edu/settings
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib3fbd684f1732a276d6033bf38507df41d33b843
Gerrit-Change-Number: 17589
Gerrit-PatchSet: 1
Gerrit-Owner: Wail Alkowaileet <[email protected]>
Gerrit-MessageType: newchange
