Move RowExpression translation above AddExchange

presto-main/src/main/java/com/facebook/presto/cost/FilterStatsCalculator.java

@@ -33,7 +33,7 @@
 import com.facebook.presto.sql.planner.ExpressionInterpreter;
 import com.facebook.presto.sql.planner.LiteralEncoder;
 import com.facebook.presto.sql.planner.LiteralInterpreter;
-import com.facebook.presto.sql.planner.NoOpSymbolResolver;
+import com.facebook.presto.sql.planner.NoOpVariableResolver;
 import com.facebook.presto.sql.planner.RowExpressionInterpreter;
 import com.facebook.presto.sql.planner.TypeProvider;
 import com.facebook.presto.sql.relational.FunctionResolution;
@@ -142,7 +142,7 @@ private Expression simplifyExpression(Session session, Expression predicate, Typ
 
         Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(session, predicate, types);
         ExpressionInterpreter interpreter = ExpressionInterpreter.expressionOptimizer(predicate, metadata, session, expressionTypes);
-        Object value = interpreter.optimize(NoOpSymbolResolver.INSTANCE);
+        Object value = interpreter.optimize(NoOpVariableResolver.INSTANCE);
 
         if (value == null) {
             // Expression evaluates to SQL null, which in Filter is equivalent to false. This assumes the expression is a top-level expression (eg. not in NOT).

presto-main/src/main/java/com/facebook/presto/cost/ScalarStatsCalculator.java

@@ -31,7 +31,7 @@
 import com.facebook.presto.sql.analyzer.ExpressionAnalyzer;
 import com.facebook.presto.sql.analyzer.Scope;
 import com.facebook.presto.sql.planner.ExpressionInterpreter;
-import com.facebook.presto.sql.planner.NoOpSymbolResolver;
+import com.facebook.presto.sql.planner.NoOpVariableResolver;
 import com.facebook.presto.sql.planner.TypeProvider;
 import com.facebook.presto.sql.relational.FunctionResolution;
 import com.facebook.presto.sql.relational.RowExpressionOptimizer;
@@ -367,7 +367,7 @@ protected VariableStatsEstimate visitFunctionCall(FunctionCall node, Void contex
         {
             Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(session, node, types);
             ExpressionInterpreter interpreter = ExpressionInterpreter.expressionOptimizer(node, metadata, session, expressionTypes);
-            Object value = interpreter.optimize(NoOpSymbolResolver.INSTANCE);
+            Object value = interpreter.optimize(NoOpVariableResolver.INSTANCE);
 
             if (value == null || value instanceof NullLiteral) {
                 return nullStatsEstimate();

presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionDomainTranslator.java

@@ -450,8 +450,8 @@ else if (symbolExpression instanceof Cast) {
         private Optional<NormalizedSimpleComparison> toNormalizedSimpleComparison(ComparisonExpression comparison)
         {
             Map<NodeRef<Expression>, Type> expressionTypes = analyzeExpression(comparison);
-            Object left = ExpressionInterpreter.expressionOptimizer(comparison.getLeft(), metadata, session, expressionTypes).optimize(NoOpSymbolResolver.INSTANCE);
-            Object right = ExpressionInterpreter.expressionOptimizer(comparison.getRight(), metadata, session, expressionTypes).optimize(NoOpSymbolResolver.INSTANCE);
+            Object left = ExpressionInterpreter.expressionOptimizer(comparison.getLeft(), metadata, session, expressionTypes).optimize(NoOpVariableResolver.INSTANCE);
+            Object right = ExpressionInterpreter.expressionOptimizer(comparison.getRight(), metadata, session, expressionTypes).optimize(NoOpVariableResolver.INSTANCE);
 
             Type leftType = expressionTypes.get(NodeRef.of(comparison.getLeft()));
             Type rightType = expressionTypes.get(NodeRef.of(comparison.getRight()));

presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java

@@ -41,7 +41,7 @@
 import com.facebook.presto.sql.analyzer.Scope;
 import com.facebook.presto.sql.analyzer.SemanticErrorCode;
 import com.facebook.presto.sql.analyzer.SemanticException;
-import com.facebook.presto.sql.planner.Interpreters.LambdaSymbolResolver;
+import com.facebook.presto.sql.planner.Interpreters.LambdaVariableResolver;
 import com.facebook.presto.sql.planner.iterative.rule.DesugarCurrentUser;
 import com.facebook.presto.sql.tree.ArithmeticBinaryExpression;
 import com.facebook.presto.sql.tree.ArithmeticUnaryExpression;
@@ -127,6 +127,7 @@
 import static com.facebook.presto.sql.planner.LiteralEncoder.estimatedSizeInBytes;
 import static com.facebook.presto.sql.planner.LiteralEncoder.isSupportedLiteralType;
 import static com.facebook.presto.sql.planner.iterative.rule.CanonicalizeExpressionRewriter.canonicalizeExpression;
+import static com.facebook.presto.sql.relational.Expressions.variable;
 import static com.facebook.presto.type.LikeFunctions.isLikePattern;
 import static com.facebook.presto.type.LikeFunctions.unescapeLiteralLikePattern;
 import static com.facebook.presto.util.LegacyRowFieldOrdinalAccessUtil.parseAnonymousRowFieldOrdinalAccess;
@@ -260,13 +261,13 @@ public Object evaluate()
         return visitor.process(expression, null);
     }
 
-    public Object evaluate(SymbolResolver inputs)
+    public Object evaluate(VariableResolver inputs)
     {
         checkState(!optimize, "evaluate(SymbolResolver) not allowed for optimizer");
         return visitor.process(expression, inputs);
     }
 
-    public Object optimize(SymbolResolver inputs)
+    public Object optimize(VariableResolver inputs)
     {
         checkState(optimize, "evaluate(SymbolResolver) not allowed for interpreter");
         return visitor.process(expression, inputs);
@@ -333,7 +334,7 @@ protected Object visitIdentifier(Identifier node, Object context)
             // ExpressionInterpreter should only be invoked after planning.
             // As a result, this method should be unreachable.
             // However, RelationPlanner.visitUnnest and visitValues invokes evaluateConstantExpression.
-            return ((SymbolResolver) context).getValue(new Symbol(node.getValue()));
+            return ((VariableResolver) context).getValue(variable(node.getValue(), type(node)));
         }
 
         @Override
@@ -345,7 +346,7 @@ protected Object visitParameter(Parameter node, Object context)
         @Override
         protected Object visitSymbolReference(SymbolReference node, Object context)
         {
-            return ((SymbolResolver) context).getValue(Symbol.from(node));
+            return ((VariableResolver) context).getValue(variable(node.getName(), type(node)));
         }
 
         @Override
@@ -935,7 +936,7 @@ protected Object visitLambdaExpression(LambdaExpression node, Object context)
                             .map(Primitives::wrap)
                             .collect(toImmutableList()),
                     argumentNames,
-                    map -> process(body, new LambdaSymbolResolver(map)));
+                    map -> process(body, new LambdaVariableResolver(map)));
         }
 
         @Override

presto-main/src/main/java/com/facebook/presto/sql/planner/Interpreters.java

@@ -14,6 +14,7 @@
 package com.facebook.presto.sql.planner;
 
 import com.facebook.presto.spi.block.Block;
+import com.facebook.presto.spi.relation.VariableReferenceExpression;
 import com.facebook.presto.spi.type.CharType;
 import com.facebook.presto.spi.type.Type;
 import com.facebook.presto.spi.type.VarcharType;
@@ -67,21 +68,21 @@ static boolean interpretLikePredicate(Type valueType, Slice value, Regex regex)
         return LikeFunctions.likeChar((long) ((CharType) valueType).getLength(), value, regex);
     }
 
-    public static class LambdaSymbolResolver
-            implements SymbolResolver
+    public static class LambdaVariableResolver
+            implements VariableResolver
     {
         private final Map<String, Object> values;
 
-        public LambdaSymbolResolver(Map<String, Object> values)
+        public LambdaVariableResolver(Map<String, Object> values)
         {
             this.values = requireNonNull(values, "values is null");
         }
 
         @Override
-        public Object getValue(Symbol symbol)
+        public Object getValue(VariableReferenceExpression variable)
         {
-            checkState(values.containsKey(symbol.getName()), "values does not contain %s", symbol);
-            return values.get(symbol.getName());
+            checkState(values.containsKey(variable.getName()), "values does not contain %s", variable);
+            return values.get(variable.getName());
         }
     }
 }

presto-main/src/main/java/com/facebook/presto/sql/planner/NoOpSymbolResolver.java → presto-main/src/main/java/com/facebook/presto/sql/planner/NoOpVariableResolver.java

@@ -13,14 +13,16 @@
  */
 package com.facebook.presto.sql.planner;
 
-public class NoOpSymbolResolver
-        implements SymbolResolver
+import com.facebook.presto.spi.relation.VariableReferenceExpression;
+
+public class NoOpVariableResolver
+        implements VariableResolver
 {
-    public static final NoOpSymbolResolver INSTANCE = new NoOpSymbolResolver();
+    public static final NoOpVariableResolver INSTANCE = new NoOpVariableResolver();
 
     @Override
-    public Object getValue(Symbol symbol)
+    public Object getValue(VariableReferenceExpression variable)
     {
-        return symbol.toSymbolReference();
+        return new Symbol(variable.getName()).toSymbolReference();
     }
 }

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java

@@ -455,6 +455,15 @@ public PlanOptimizers(
                         .add(new InlineProjections(metadata.getFunctionManager()))
                         .build()));
 
+        // TODO: move this before optimization if possible!!
+        // Replace all expressions with row expressions
+        builder.add(new IterativeOptimizer(
+                ruleStats,
+                statsCalculator,
+                costCalculator,
+                new TranslateExpressions(metadata, sqlParser).rules()));
+        // After this point, all planNodes should not contain OriginalExpression
+
         if (!forceSingleNode) {
             builder.add(new ReplicateSemiJoinInDelete()); // Must run before AddExchanges
             builder.add((new IterativeOptimizer(
@@ -475,15 +484,6 @@ public PlanOptimizers(
             builder.add(new StatsRecordingPlanOptimizer(optimizerStats, new AddExchanges(metadata, sqlParser)));
         }
 
-        // TODO: move this before optimization if possible!!
-        // Replace all expressions with row expressions
-        builder.add(new IterativeOptimizer(
-                ruleStats,
-                statsCalculator,
-                costCalculator,
-                new TranslateExpressions(metadata, sqlParser).rules()));
-        // After this point, all planNodes should not contain OriginalExpression
-
         //noinspection UnusedAssignment
         estimatedExchangesCostCalculator = null; // Prevent accidental use after AddExchanges
 

presto-main/src/main/java/com/facebook/presto/sql/planner/RowExpressionInterpreter.java

@@ -39,11 +39,10 @@
 import com.facebook.presto.spi.type.TypeManager;
 import com.facebook.presto.spi.type.TypeSignature;
 import com.facebook.presto.sql.InterpretedFunctionInvoker;
-import com.facebook.presto.sql.planner.Interpreters.LambdaSymbolResolver;
+import com.facebook.presto.sql.planner.Interpreters.LambdaVariableResolver;
 import com.facebook.presto.sql.relational.FunctionResolution;
 import com.facebook.presto.sql.relational.RowExpressionDeterminismEvaluator;
 import com.facebook.presto.util.Failures;
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.primitives.Primitives;
 import io.airlift.joni.Regex;
@@ -171,10 +170,9 @@ public Object optimize()
     }
 
     /**
-     * For test only; convenient to replace symbol with constants. Production code should not replace any symbols; use the interface above
+     * Replace symbol with constants
      */
-    @VisibleForTesting
-    public Object optimize(SymbolResolver inputs)
+    public Object optimize(VariableResolver inputs)
     {
         checkState(optimizationLevel.ordinal() <= EVALUATED.ordinal(), "optimize(SymbolResolver) not allowed for interpreter");
         return expression.accept(visitor, inputs);
@@ -198,8 +196,8 @@ public Object visitConstant(ConstantExpression node, Object context)
         @Override
         public Object visitVariableReference(VariableReferenceExpression node, Object context)
         {
-            if (context instanceof SymbolResolver) {
-                return ((SymbolResolver) context).getValue(new Symbol(node.getName()));
+            if (context instanceof VariableResolver) {
+                return ((VariableResolver) context).getValue(node);
             }
             return node;
         }
@@ -281,7 +279,7 @@ public Object visitLambda(LambdaDefinitionExpression node, Object context)
                             .map(Primitives::wrap)
                             .collect(toImmutableList()),
                     node.getArguments(),
-                    map -> body.accept(this, new LambdaSymbolResolver(map)));
+                    map -> body.accept(this, new LambdaVariableResolver(map)));
         }
 
         @Override

presto-main/src/main/java/com/facebook/presto/sql/planner/SymbolResolver.java → presto-main/src/main/java/com/facebook/presto/sql/planner/VariableResolver.java

@@ -13,7 +13,9 @@
  */
 package com.facebook.presto.sql.planner;
 
-public interface SymbolResolver
+import com.facebook.presto.spi.relation.VariableReferenceExpression;
+
+public interface VariableResolver
 {
-    Object getValue(Symbol symbol);
+    Object getValue(VariableReferenceExpression variable);
 }