IMPALA-13136: Refactor AnalyzedFunctionCallExpr (for Calcite)

The analyze method is now called after the Expr is constructed.

This code is more in line with the existing way that Impala
constructs the Expr object.

Change-Id: Ideb662d9c7536659cb558bf62baec29c82217aa2
Reviewed-on: http://gerrit.cloudera.org:8080/21525
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Reviewed-by: Joe McDonnell <joemcdonnell@cloudera.com>

Author: scarlin-cloudera

java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java

@@ -31,14 +31,12 @@
 import java.util.List;
 
 /**
- * A FunctionCallExpr that is always in an analyzed state
+ * A FunctionCallExpr specialized for Calcite.
  *
- * The analysis for Calcite expressions can be done in the constructor
- * rather than issuing a separate call to "analyze" after the object
- * is constructed.
- *
- * For this class, we also want to override the "analyzeImpl" call since
- * the "fn_" member is passed in rather than deduced at analysis time.
+ * The analysis for Calcite expressions is done through Calcite and
+ * does not need the analysis provided through the Impala expression.
+ * The analyzeImpl is overridden for FunctionCallExpr and only does
+ * the minimal analysis needed.
  *
  */
 public class AnalyzedFunctionCallExpr extends FunctionCallExpr {
@@ -49,40 +47,27 @@ public class AnalyzedFunctionCallExpr extends FunctionCallExpr {
   // variable so it can be properly set in analyzeImpl()
   private final Function savedFunction_;
 
-  private final Analyzer analyzer_;
-
   // c'tor that takes a list of Exprs that eventually get converted to FunctionParams
   public AnalyzedFunctionCallExpr(Function fn, List<Expr> params,
-      RexCall rexCall, Type retType, Analyzer analyzer) throws ImpalaException {
+      RexCall rexCall, Type retType) {
     super(fn.getFunctionName(), params);
     this.savedFunction_ = fn;
     this.type_ = retType;
-    this.analyze(analyzer);
-    this.analyzer_ = analyzer;
   }
 
   // c'tor which does not depend on Calcite's RexCall but is used when Impala's
   // FunctionParams are created or there is some modifications to it
   public AnalyzedFunctionCallExpr(Function fn, FunctionParams funcParams,
-      Type retType, Analyzer analyzer) throws ImpalaException {
+      Type retType) {
     super(fn.getFunctionName(), funcParams);
     this.savedFunction_ = fn;
     this.type_ = retType;
-    this.analyze(analyzer);
-    this.analyzer_ = analyzer;
   }
 
   public AnalyzedFunctionCallExpr(AnalyzedFunctionCallExpr other) {
     super(other);
     this.savedFunction_ = other.savedFunction_;
     this.type_ = other.type_;
-    this.analyzer_ = other.analyzer_;
-    try {
-      this.analyze(this.analyzer_);
-    } catch (ImpalaException e) {
-      //TODO: IMPALA-13097: Don't throw runtime exception
-      throw new RuntimeException(e);
-    }
   }
 
   @Override
@@ -106,12 +91,8 @@ protected float computeEvalCost() {
 
   @Override
   public FunctionCallExpr cloneWithNewParams(FunctionParams params) {
-    try {
-      return new AnalyzedFunctionCallExpr(this.getFn(), params,
-          this.type_, analyzer_);
-    } catch (ImpalaException e) {
-      throw new IllegalStateException(e);
-    }
+    return new AnalyzedFunctionCallExpr(this.getFn(), params,
+        this.type_);
   }
 
 }

java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java

@@ -25,22 +25,21 @@
 import org.apache.impala.common.ImpalaException;
 
 /**
- * A NulLLiteral that is always in analyzed state
+ * A NulLLiteral specialized for Calcite
+ *
+ * The analysisImpl is overriden for Calcite so that the NullLiteral
+ * can be cast to the type that was determined by Calcite.
  */
 public class AnalyzedNullLiteral extends NullLiteral {
-  private final Analyzer analyzer_;
 
   private final Type savedType_;
 
-  public AnalyzedNullLiteral(Analyzer analyzer, Type type) throws ImpalaException {
-    this.analyzer_ = analyzer;
+  public AnalyzedNullLiteral(Type type) {
     savedType_ = type;
-    this.analyze(analyzer);
   }
 
   public AnalyzedNullLiteral(AnalyzedNullLiteral other) {
     super(other);
-    this.analyzer_ = other.analyzer_;
     this.savedType_ = other.savedType_;
   }
 

java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java

@@ -17,12 +17,12 @@
 
 package org.apache.impala.calcite.functions;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rex.RexBuilder;
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.rex.RexNode;
-import org.apache.impala.analysis.Analyzer;
 import org.apache.impala.analysis.Expr;
 import org.apache.impala.calcite.type.ImpalaTypeConverter;
 import org.apache.impala.catalog.Function;
@@ -32,6 +32,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+
 import java.util.List;
 
 /**
@@ -44,8 +45,7 @@ public class RexCallConverter {
   /*
    * Returns the Impala Expr object for RexCallConverter.
    */
-  public static Expr getExpr(RexCall rexCall, List<Expr> params,
-      RexBuilder rexBuilder, Analyzer analyzer) throws ImpalaException {
+  public static Expr getExpr(RexCall rexCall, List<Expr> params, RexBuilder rexBuilder) {
 
     String funcName = rexCall.getOperator().getName().toLowerCase();
 
@@ -54,17 +54,15 @@ public static Expr getExpr(RexCall rexCall, List<Expr> params,
     Type impalaRetType = ImpalaTypeConverter.createImpalaType(fn.getReturnType(),
         rexCall.getType().getPrecision(), rexCall.getType().getScale());
 
-    return new AnalyzedFunctionCallExpr(fn, params, rexCall, impalaRetType, analyzer);
+    return new AnalyzedFunctionCallExpr(fn, params, rexCall, impalaRetType);
   }
 
-  private static Function getFunction(RexCall call) throws ImpalaException {
+  private static Function getFunction(RexCall call) {
     List<RelDataType> argTypes = Lists.transform(call.getOperands(), RexNode::getType);
     String name = call.getOperator().getName();
     Function fn = FunctionResolver.getFunction(name, call.getKind(), argTypes);
-    if (fn == null) {
-      throw new AnalysisException("Could not find function \"" + name + "\" in Impala "
+    Preconditions.checkNotNull(fn, "Could not find function \"" + name + "\" in Impala "
           + "with args " + argTypes + " and return type " + call.getType());
-    }
     return fn;
   }
 }

java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java

@@ -24,7 +24,6 @@
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.util.DateString;
 import org.apache.calcite.util.TimestampString;
-import org.apache.impala.analysis.Analyzer;
 import org.apache.impala.analysis.Expr;
 import org.apache.impala.analysis.BoolLiteral;
 import org.apache.impala.analysis.DateLiteral;
@@ -55,26 +54,23 @@ public class RexLiteralConverter {
   /*
    * Returns Expr object for ImpalaRexLiteral
    */
-  public static Expr getExpr(RexLiteral rexLiteral, Analyzer analyzer)
-      throws ImpalaException {
+  public static Expr getExpr(RexLiteral rexLiteral) {
     if (SqlTypeName.INTERVAL_TYPES.contains(rexLiteral.getTypeName())) {
-      return new NumericLiteral(
+      return NumericLiteral.create(
           new BigDecimal(rexLiteral.getValueAs(Long.class)), Type.BIGINT);
     }
     switch (rexLiteral.getTypeName()) {
       case NULL:
         Type type = ImpalaTypeConverter.createImpalaType(rexLiteral.getType());
-        return new AnalyzedNullLiteral(null, type);
+        return new AnalyzedNullLiteral(type);
       case BOOLEAN:
-        Expr boolExpr = new BoolLiteral(rexLiteral.getValueAs(Boolean.class));
-        boolExpr.analyze(null);
+        Expr boolExpr = new BoolLiteral((Boolean) rexLiteral.getValueAs(Boolean.class));
         return boolExpr;
       case BIGINT:
       case DECIMAL:
       case DOUBLE:
-        Expr numericExpr = new NumericLiteral(rexLiteral.getValueAs(BigDecimal.class),
+        Expr numericExpr = NumericLiteral.create(rexLiteral.getValueAs(BigDecimal.class),
             ImpalaTypeConverter.createImpalaType(rexLiteral.getType()));
-        numericExpr.analyze(null);
         return numericExpr;
       case CHAR:
       case VARCHAR:
@@ -87,19 +83,18 @@ public static Expr getExpr(RexLiteral rexLiteral, Analyzer analyzer)
         }
         Expr charExpr = new StringLiteral(rexLiteral.getValueAs(String.class),
             charType, false);
-        charExpr.analyze(null);
         return charExpr;
       case DATE:
         DateString dateStringClass = rexLiteral.getValueAs(DateString.class);
         String dateString = (dateStringClass == null) ? null : dateStringClass.toString();
         Expr dateExpr = new DateLiteral(rexLiteral.getValueAs(Integer.class), dateString);
-        dateExpr.analyze(null);
         return dateExpr;
       case TIMESTAMP:
-          return createCastTimestampExpr(rexLiteral, analyzer);
+          return createCastTimestampExpr(rexLiteral);
       default:
-        throw new AnalysisException("Unsupported RexLiteral: "
+        Preconditions.checkState(false, "Unsupported RexLiteral: "
             + rexLiteral.getTypeName());
+        return null;
     }
   }
 
@@ -110,17 +105,14 @@ public static Expr getExpr(RexLiteral rexLiteral, Analyzer analyzer)
    * If constant folding was not allowed, it means we did not have access to the backend
    * and thus need to do a cast in order to support conversion to a Timestamp.
    */
-  private static Expr createCastTimestampExpr(RexLiteral rexLiteral,
-      Analyzer analyzer)
-      throws ImpalaException {
+  private static Expr createCastTimestampExpr(RexLiteral rexLiteral) {
     List<RelDataType> typeNames =
         ImmutableList.of(ImpalaTypeConverter.getRelDataType(Type.STRING));
 
     String timestamp = rexLiteral.getValueAs(TimestampString.class).toString();
     List<Expr> argList =
         Lists.newArrayList(new StringLiteral(timestamp, Type.STRING, false));
     Function castFunc = FunctionResolver.getFunction("casttotimestamp", typeNames);
-    return new AnalyzedFunctionCallExpr(castFunc, argList, null, Type.TIMESTAMP,
-        analyzer);
+    return new AnalyzedFunctionCallExpr(castFunc, argList, null, Type.TIMESTAMP);
   }
 }

java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java

@@ -70,24 +70,16 @@ public Expr visitInputRef(RexInputRef rexInputRef) {
 
   @Override
   public Expr visitCall(RexCall rexCall) {
-    try {
-      List<Expr> params = Lists.newArrayList();
-      for (RexNode operand : rexCall.getOperands()) {
-        params.add(operand.accept(this));
-      }
-      return RexCallConverter.getExpr(rexCall, params, rexBuilder_, analyzer_);
-    } catch (ImpalaException e) {
-      throw new RuntimeException(e);
+    List<Expr> params = Lists.newArrayList();
+    for (RexNode operand : rexCall.getOperands()) {
+      params.add(operand.accept(this));
     }
+    return RexCallConverter.getExpr(rexCall, params, rexBuilder_);
   }
 
   @Override
   public Expr visitLiteral(RexLiteral rexLiteral) {
-    try {
-      return RexLiteralConverter.getExpr(rexLiteral, analyzer_);
-    } catch (ImpalaException e) {
-      throw new RuntimeException(e);
-    }
+    return RexLiteralConverter.getExpr(rexLiteral);
   }
 
   @Override
@@ -142,7 +134,9 @@ public Expr visitPatternFieldRef(RexPatternFieldRef fieldRef) {
   public static Expr getExpr(CreateExprVisitor visitor, RexNode operand)
       throws ImpalaException {
     try {
-      return operand.accept(visitor);
+      Expr expr = operand.accept(visitor);
+      expr.analyze(visitor.analyzer_);
+      return expr;
     } catch (Exception e) {
       throw new AnalysisException(e);
     }