impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From tarmstr...@apache.org
Subject incubator-impala git commit: IMPALA-4617: remove IsConstant() analysis from be
Date Tue, 31 Jan 2017 18:09:31 GMT
Repository: incubator-impala
Updated Branches:
  refs/heads/master a4206d3f1 -> a4eb4705c


IMPALA-4617: remove IsConstant() analysis from be

This change avoids the need to duplicate the logic in Expr.getConstant()
in the frontend and Expr::GetConstant() in the backend. Instead it is
plumbed through from the frontend.

To make it easier to reason about the state of isAnalyzed_ and
isConstant_, made isAnalyzed_ private and refactored
analyze() so that isAnalyzed_ is only set at the end of analysis, not in
the middle of it.

I considered going further and only allowing isConstant() to be called
only on analyzed expressions, but there are multiple places in the code
that depend on this working in non-trivial ways, so that would be a lot
more invasive.

There should be no behavioural changes as a result of this patch, aside
from a minor fix for "limit count(*)" where an internal error message
was produced instead of the expected one about constant expressions.

Testing:
Ran exhaustive tests. Added a regression test for "limit count(*)".

Change-Id: I24b4c9d6e8e57fd73d906a9c36200e1a84497b90
Reviewed-on: http://gerrit.cloudera.org:8080/5415
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/a4eb4705
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/a4eb4705
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/a4eb4705

Branch: refs/heads/master
Commit: a4eb4705c3796b1129b33cc99dd96ee8d8bddafb
Parents: a4206d3
Author: Tim Armstrong <tarmstrong@cloudera.com>
Authored: Fri Dec 16 07:41:43 2016 -0800
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Tue Jan 31 10:27:20 2017 +0000

----------------------------------------------------------------------
 be/src/exec/hdfs-table-sink.cc                  |  4 +-
 be/src/exprs/expr.cc                            | 18 +++---
 be/src/exprs/expr.h                             | 27 ++++-----
 be/src/exprs/literal.cc                         | 20 +++---
 be/src/exprs/null-literal.h                     |  2 +-
 be/src/exprs/scalar-fn-call.cc                  | 16 ++---
 be/src/exprs/scalar-fn-call.h                   |  4 --
 be/src/exprs/slot-ref.cc                        |  6 +-
 be/src/exprs/slot-ref.h                         |  5 +-
 be/src/exprs/tuple-is-null-predicate.h          |  2 -
 common/thrift/Exprs.thrift                      | 37 +++++------
 .../apache/impala/analysis/AnalyticExpr.java    |  6 +-
 .../apache/impala/analysis/AnalyticInfo.java    |  2 +-
 .../org/apache/impala/analysis/Analyzer.java    |  4 +-
 .../apache/impala/analysis/ArithmeticExpr.java  |  4 +-
 .../impala/analysis/BetweenPredicate.java       |  6 +-
 .../apache/impala/analysis/BinaryPredicate.java |  6 +-
 .../org/apache/impala/analysis/CaseExpr.java    |  5 +-
 .../org/apache/impala/analysis/CastExpr.java    |  6 +-
 .../impala/analysis/CompoundPredicate.java      |  6 +-
 .../apache/impala/analysis/ExistsPredicate.java |  6 --
 .../java/org/apache/impala/analysis/Expr.java   | 64 +++++++++++++++++---
 .../impala/analysis/ExprSubstitutionMap.java    |  4 +-
 .../apache/impala/analysis/ExtractFromExpr.java |  8 ++-
 .../impala/analysis/FunctionCallExpr.java       | 19 +++---
 .../org/apache/impala/analysis/InPredicate.java |  6 +-
 .../impala/analysis/IsNotEmptyPredicate.java    |  5 +-
 .../apache/impala/analysis/IsNullPredicate.java |  6 +-
 .../apache/impala/analysis/LikePredicate.java   |  5 +-
 .../apache/impala/analysis/LimitElement.java    | 42 ++++++-------
 .../org/apache/impala/analysis/LiteralExpr.java |  5 ++
 .../apache/impala/analysis/NumericLiteral.java  | 20 ++----
 .../org/apache/impala/analysis/Predicate.java   |  4 +-
 .../org/apache/impala/analysis/SlotRef.java     | 18 +++---
 .../org/apache/impala/analysis/Subquery.java    |  7 +--
 .../analysis/TimestampArithmeticExpr.java       |  5 +-
 .../impala/analysis/TupleIsNullPredicate.java   |  7 +--
 .../apache/impala/analysis/AnalyzerTest.java    |  5 ++
 38 files changed, 203 insertions(+), 219 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exec/hdfs-table-sink.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-table-sink.cc b/be/src/exec/hdfs-table-sink.cc
index 3a9725c..ddda05b 100644
--- a/be/src/exec/hdfs-table-sink.cc
+++ b/be/src/exec/hdfs-table-sink.cc
@@ -93,7 +93,7 @@ Status HdfsTableSink::PrepareExprs(RuntimeState* state) {
   // Prepare partition key exprs and gather dynamic partition key exprs.
   for (size_t i = 0; i < partition_key_expr_ctxs_.size(); ++i) {
     // Remember non-constant partition key exprs for building hash table of Hdfs files.
-    if (!partition_key_expr_ctxs_[i]->root()->IsConstant()) {
+    if (!partition_key_expr_ctxs_[i]->root()->is_constant()) {
       dynamic_partition_key_expr_ctxs_.push_back(partition_key_expr_ctxs_[i]);
     }
   }
@@ -185,7 +185,7 @@ Status HdfsTableSink::Open(RuntimeState* state) {
       vector<ExprContext*> dynamic_partition_key_value_ctxs;
       for (size_t i = 0; i < partition_key_expr_ctxs_.size(); ++i) {
         // Remember non-constant partition key exprs for building hash table of Hdfs files
-        if (!partition_key_expr_ctxs_[i]->root()->IsConstant()) {
+        if (!partition_key_expr_ctxs_[i]->root()->is_constant()) {
           dynamic_partition_key_value_ctxs.push_back(
               partition->partition_key_value_ctxs()[i]);
         } else {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/expr.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr.cc b/be/src/exprs/expr.cc
index e0f9516..2ffddf6 100644
--- a/be/src/exprs/expr.cc
+++ b/be/src/exprs/expr.cc
@@ -99,8 +99,9 @@ FunctionContext* Expr::RegisterFunctionContext(ExprContext* ctx, RuntimeState* s
   return ctx->fn_context(fn_context_index_);
 }
 
-Expr::Expr(const ColumnType& type, bool is_slotref)
+Expr::Expr(const ColumnType& type, bool is_constant, bool is_slotref)
     : cache_entry_(NULL),
+      is_constant_(is_constant),
       is_slotref_(is_slotref),
       type_(type),
       output_scale_(-1),
@@ -110,6 +111,7 @@ Expr::Expr(const ColumnType& type, bool is_slotref)
 
 Expr::Expr(const TExprNode& node, bool is_slotref)
     : cache_entry_(NULL),
+      is_constant_(node.is_constant),
       is_slotref_(is_slotref),
       type_(ColumnType::FromThrift(node.type)),
       output_scale_(-1),
@@ -446,13 +448,6 @@ string Expr::DebugString(const vector<ExprContext*>& ctxs) {
   return DebugString(exprs);
 }
 
-bool Expr::IsConstant() const {
-  for (int i = 0; i < children_.size(); ++i) {
-    if (!children_[i]->IsConstant()) return false;
-  }
-  return true;
-}
-
 bool Expr::IsLiteral() const {
   return false;
 }
@@ -532,9 +527,10 @@ void Expr::InitBuiltinsDummy() {
   UtilityFunctions::Pid(NULL);
 }
 
-Status Expr::GetConstVal(RuntimeState* state, ExprContext* context, AnyVal** const_val) {
+Status Expr::GetConstVal(
+    RuntimeState* state, ExprContext* context, AnyVal** const_val) {
   DCHECK(context->opened_);
-  if (!IsConstant()) {
+  if (!is_constant()) {
     *const_val = NULL;
     return Status::OK();
   }
@@ -589,7 +585,7 @@ Status Expr::GetConstVal(RuntimeState* state, ExprContext* context, AnyVal** con
     default:
       DCHECK(false) << "Type not implemented: " << type();
   }
-  // Errors may have been set during the GetConstVal() call.
+  // Errors may have been set during expr evaluation.
   return GetFnContextError(context);
 }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/expr.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr.h b/be/src/exprs/expr.h
index 1eee396..b77c9a2 100644
--- a/be/src/exprs/expr.h
+++ b/be/src/exprs/expr.h
@@ -158,6 +158,7 @@ class Expr {
 
   const ColumnType& type() const { return type_; }
   bool is_slotref() const { return is_slotref_; }
+  bool is_constant() const { return is_constant_; }
 
   const std::vector<Expr*>& children() const { return children_; }
 
@@ -165,13 +166,6 @@ class Expr {
   /// expr has an error set.
   Status GetFnContextError(ExprContext* ctx);
 
-  /// Returns true if the expression is considered constant. This must match the
-  /// definition of Expr.isConstant() in the frontend. The default implementation returns
-  /// true if all children are constant.
-  /// TODO: IMPALA-4617 - plumb through the value from the frontend and remove duplicate
-  /// logic.
-  virtual bool IsConstant() const;
-
   /// Returns true if this is a literal expression.
   virtual bool IsLiteral() const;
 
@@ -247,12 +241,12 @@ class Expr {
   /// appropriate type of AnyVal.
   virtual Status GetCodegendComputeFn(LlvmCodeGen* codegen, llvm::Function** fn) = 0;
 
-  /// If this expr is constant, evaluates the expr with no input row argument and returns
-  /// the result in 'const_val'. Sets 'const_val' to NULL if the argument is not constant.
-  /// The returned AnyVal and associated varlen data is owned by 'context'. This should
-  /// only be called after Open() has been called on this expr. Returns an error if there
-  /// was an error evaluating the expression or if memory could not be allocated for the
-  /// expression result.
+  /// If this expr is constant according to is_constant(), evaluates the expr with no
+  /// input row argument and returns the result in 'const_val'. Otherwise sets
+  /// 'const_val' to nullptr. The returned AnyVal and associated varlen data is owned by
+  /// 'context'. This should only be called after Open() has been called on this expr.
+  /// Returns an error if there was an error evaluating the expression or if memory could
+  /// not be allocated for the expression result.
   virtual Status GetConstVal(
       RuntimeState* state, ExprContext* context, AnyVal** const_val);
 
@@ -329,7 +323,7 @@ class Expr {
   friend class FunctionCall;
   friend class ScalarFnCall;
 
-  Expr(const ColumnType& type, bool is_slotref = false);
+  Expr(const ColumnType& type, bool is_constant, bool is_slotref);
   Expr(const TExprNode& node, bool is_slotref = false);
 
   /// Initializes this expr instance for execution. This does not include initializing
@@ -365,6 +359,11 @@ class Expr {
   /// Function description.
   TFunction fn_;
 
+  /// True if this expr should be treated as a constant expression. True if either:
+  /// * This expr was sent from the frontend and Expr.isConstant() was true.
+  /// * This expr is a constant literal created in the backend.
+  const bool is_constant_;
+
   /// recognize if this node is a slotref in order to speed up GetValue()
   const bool is_slotref_;
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/literal.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/literal.cc b/be/src/exprs/literal.cc
index 0445f64..32f8250 100644
--- a/be/src/exprs/literal.cc
+++ b/be/src/exprs/literal.cc
@@ -137,43 +137,43 @@ Literal::Literal(const TExprNode& node)
 }
 
 Literal::Literal(ColumnType type, bool v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_BOOLEAN) << type;
   value_.bool_val = v;
 }
 
 Literal::Literal(ColumnType type, int8_t v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_TINYINT) << type;
   value_.tinyint_val = v;
 }
 
 Literal::Literal(ColumnType type, int16_t v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_SMALLINT) << type;
   value_.smallint_val = v;
 }
 
 Literal::Literal(ColumnType type, int32_t v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_INT) << type;
   value_.int_val = v;
 }
 
 Literal::Literal(ColumnType type, int64_t v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_BIGINT) << type;
   value_.bigint_val = v;
 }
 
 Literal::Literal(ColumnType type, float v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_FLOAT) << type;
   value_.float_val = v;
 }
 
 Literal::Literal(ColumnType type, double v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   if (type.type == TYPE_DOUBLE) {
     value_.double_val = v;
   } else if (type.type == TYPE_TIMESTAMP) {
@@ -197,19 +197,19 @@ Literal::Literal(ColumnType type, double v)
   }
 }
 
-Literal::Literal(ColumnType type, const string& v) : Expr(type) {
+Literal::Literal(ColumnType type, const string& v) : Expr(type, true, false) {
   value_.Init(v);
   DCHECK(type.type == TYPE_STRING || type.type == TYPE_CHAR || type.type == TYPE_VARCHAR)
       << type;
 }
 
-Literal::Literal(ColumnType type, const StringValue& v) : Expr(type) {
+Literal::Literal(ColumnType type, const StringValue& v) : Expr(type, true, false) {
   value_.Init(v.DebugString());
   DCHECK(type.type == TYPE_STRING || type.type == TYPE_CHAR) << type;
 }
 
 Literal::Literal(ColumnType type, const TimestampValue& v)
-  : Expr(type) {
+  : Expr(type, true, false) {
   DCHECK_EQ(type.type, TYPE_TIMESTAMP) << type;
   value_.timestamp_val = v;
 }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/null-literal.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/null-literal.h b/be/src/exprs/null-literal.h
index d908baa..d7a495a 100644
--- a/be/src/exprs/null-literal.h
+++ b/be/src/exprs/null-literal.h
@@ -27,7 +27,7 @@ class TExprNode;
 
 class NullLiteral: public Expr {
  public:
-  NullLiteral(PrimitiveType type) : Expr(type) { }
+  NullLiteral(PrimitiveType type) : Expr(type, true, false) { }
 
   virtual bool IsLiteral() const;
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/scalar-fn-call.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-fn-call.cc b/be/src/exprs/scalar-fn-call.cc
index 44f6ecc..c7d4e7e 100644
--- a/be/src/exprs/scalar-fn-call.cc
+++ b/be/src/exprs/scalar-fn-call.cc
@@ -216,12 +216,12 @@ Status ScalarFnCall::Open(RuntimeState* state, ExprContext* ctx,
       // are evaluated. This means that setting enable_expr_rewrites=false will also
       // disable caching of non-literal constant expressions, which gives the old
       // behaviour (before this caching optimisation was added) of repeatedly evaluating
-      // exprs that are constant according to IsConstant(). For exprs that are not truly
-      // constant (yet IsConstant() returns true for) e.g. non-deterministic UDFs, this
+      // exprs that are constant according to is_constant(). For exprs that are not truly
+      // constant (yet is_constant() returns true for) e.g. non-deterministic UDFs, this
       // means that setting enable_expr_rewrites=false works as a safety valve to get
       // back the old behaviour, before constant expr folding or caching was added.
       // TODO: once we can annotate UDFs as non-deterministic (IMPALA-4606), we should
-      // be able to trust IsConstant() and switch back to that.
+      // be able to trust is_constant() and switch back to that.
       if (children_[i]->IsLiteral()) {
         const AnyVal* constant_arg = fn_ctx->impl()->constant_args()[i];
         DCHECK(constant_arg != NULL);
@@ -248,7 +248,7 @@ Status ScalarFnCall::Open(RuntimeState* state, ExprContext* ctx,
   // non-constant.
   if (fn_.name.function_name == "round" && type_.type == TYPE_DOUBLE) {
     DCHECK_EQ(children_.size(), 2);
-    if (children_[1]->IsConstant()) {
+    if (children_[1]->is_constant()) {
       IntVal scale_arg = children_[1]->GetIntVal(ctx, NULL);
       output_scale_ = scale_arg.val;
     }
@@ -269,14 +269,6 @@ void ScalarFnCall::Close(RuntimeState* state, ExprContext* context,
   Expr::Close(state, context, scope);
 }
 
-bool ScalarFnCall::IsConstant() const {
-  if (fn_.name.function_name == "rand" || fn_.name.function_name == "random"
-      || fn_.name.function_name == "uuid" || fn_.name.function_name == "sleep") {
-    return false;
-  }
-  return Expr::IsConstant();
-}
-
 // Dynamically loads the pre-compiled UDF and codegens a function that calls each child's
 // codegen'd function, then passes those values to the UDF and returns the result.
 // Example generated IR for a UDF with signature

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/scalar-fn-call.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-fn-call.h b/be/src/exprs/scalar-fn-call.h
index 1cb3743..c8bc8c8 100644
--- a/be/src/exprs/scalar-fn-call.h
+++ b/be/src/exprs/scalar-fn-call.h
@@ -65,10 +65,6 @@ class ScalarFnCall: public Expr {
   virtual void Close(RuntimeState* state, ExprContext* context,
       FunctionContext::FunctionStateScope scope = FunctionContext::FRAGMENT_LOCAL);
 
-  /// Needs to be kept in sync with the FE understanding of constness in
-  /// FuctionCallExpr.java.
-  virtual bool IsConstant() const;
-
   virtual BooleanVal GetBooleanVal(ExprContext* context, const TupleRow*);
   virtual TinyIntVal GetTinyIntVal(ExprContext* context, const TupleRow*);
   virtual SmallIntVal GetSmallIntVal(ExprContext* context, const TupleRow*);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/slot-ref.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/slot-ref.cc b/be/src/exprs/slot-ref.cc
index 22c22b6..3465ff4 100644
--- a/be/src/exprs/slot-ref.cc
+++ b/be/src/exprs/slot-ref.cc
@@ -47,7 +47,7 @@ SlotRef::SlotRef(const TExprNode& node)
 }
 
 SlotRef::SlotRef(const SlotDescriptor* desc)
-  : Expr(desc->type(), true),
+  : Expr(desc->type(), false, true),
     slot_offset_(-1),
     null_indicator_offset_(0, 0),
     slot_id_(desc->id()) {
@@ -55,7 +55,7 @@ SlotRef::SlotRef(const SlotDescriptor* desc)
 }
 
 SlotRef::SlotRef(const SlotDescriptor* desc, const ColumnType& type)
-  : Expr(type, true),
+  : Expr(type, false, true),
     slot_offset_(-1),
     null_indicator_offset_(0, 0),
     slot_id_(desc->id()) {
@@ -63,7 +63,7 @@ SlotRef::SlotRef(const SlotDescriptor* desc, const ColumnType& type)
 }
 
   SlotRef::SlotRef(const ColumnType& type, int offset, const bool nullable /* = false */)
-  : Expr(type, true),
+  : Expr(type, false, true),
     tuple_idx_(0),
     slot_offset_(offset),
     null_indicator_offset_(0, nullable ? offset : -1),

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/slot-ref.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/slot-ref.h b/be/src/exprs/slot-ref.h
index 6ca3c89..e86617d 100644
--- a/be/src/exprs/slot-ref.h
+++ b/be/src/exprs/slot-ref.h
@@ -36,10 +36,9 @@ class SlotRef : public Expr {
   /// Used for testing.  GetValue will return tuple + offset interpreted as 'type'
   SlotRef(const ColumnType& type, int offset, const bool nullable = false);
 
-  virtual Status Prepare(RuntimeState* state, const RowDescriptor& row_desc,
-                         ExprContext* context);
+  virtual Status Prepare(
+      RuntimeState* state, const RowDescriptor& row_desc, ExprContext* context);
   virtual std::string DebugString() const;
-  virtual bool IsConstant() const { return false; }
   virtual int GetSlotIds(std::vector<SlotId>* slot_ids) const;
   const SlotId& slot_id() const { return slot_id_; }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/be/src/exprs/tuple-is-null-predicate.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/tuple-is-null-predicate.h b/be/src/exprs/tuple-is-null-predicate.h
index e20a195..e687010 100644
--- a/be/src/exprs/tuple-is-null-predicate.h
+++ b/be/src/exprs/tuple-is-null-predicate.h
@@ -41,8 +41,6 @@ class TupleIsNullPredicate: public Predicate {
   virtual Status GetCodegendComputeFn(LlvmCodeGen* codegen, llvm::Function** fn);
   virtual std::string DebugString() const;
 
-  virtual bool IsConstant() const { return false; }
-
   virtual BooleanVal GetBooleanVal(ExprContext* context, const TupleRow* row);
 
  private:

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/common/thrift/Exprs.thrift
----------------------------------------------------------------------
diff --git a/common/thrift/Exprs.thrift b/common/thrift/Exprs.thrift
index 205f7bb..33b859a 100644
--- a/common/thrift/Exprs.thrift
+++ b/common/thrift/Exprs.thrift
@@ -123,26 +123,29 @@ struct TExprNode {
   2: required Types.TColumnType type
   3: required i32 num_children
 
+  // Whether the Expr is constant according to the frontend.
+  4: required bool is_constant
+
   // The function to execute. Not set for SlotRefs and Literals.
-  4: optional Types.TFunction fn
+  5: optional Types.TFunction fn
 
   // If set, child[vararg_start_idx] is the first vararg child.
-  5: optional i32 vararg_start_idx
-
-  6: optional TBoolLiteral bool_literal
-  7: optional TCaseExpr case_expr
-  8: optional TDateLiteral date_literal
-  9: optional TFloatLiteral float_literal
-  10: optional TIntLiteral int_literal
-  11: optional TInPredicate in_predicate
-  12: optional TIsNullPredicate is_null_pred
-  13: optional TLiteralPredicate literal_pred
-  14: optional TSlotRef slot_ref
-  15: optional TStringLiteral string_literal
-  16: optional TTupleIsNullPredicate tuple_is_null_pred
-  17: optional TDecimalLiteral decimal_literal
-  18: optional TAggregateExpr agg_expr
-  19: optional TTimestampLiteral timestamp_literal
+  6: optional i32 vararg_start_idx
+
+  7: optional TBoolLiteral bool_literal
+  8: optional TCaseExpr case_expr
+  9: optional TDateLiteral date_literal
+  10: optional TFloatLiteral float_literal
+  11: optional TIntLiteral int_literal
+  12: optional TInPredicate in_predicate
+  13: optional TIsNullPredicate is_null_pred
+  14: optional TLiteralPredicate literal_pred
+  15: optional TSlotRef slot_ref
+  16: optional TStringLiteral string_literal
+  17: optional TTupleIsNullPredicate tuple_is_null_pred
+  18: optional TDecimalLiteral decimal_literal
+  19: optional TAggregateExpr agg_expr
+  20: optional TTimestampLiteral timestamp_literal
 }
 
 // A flattened representation of a tree of Expr nodes, obtained by depth-first

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java b/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
index 0a51808..864c10b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
@@ -145,7 +145,7 @@ public class AnalyticExpr extends Expr {
    * Analytic exprs cannot be constant.
    */
   @Override
-  public boolean isConstant() { return false; }
+  protected boolean isConstantImpl() { return false; }
 
   @Override
   public Expr clone() { return new AnalyticExpr(this); }
@@ -417,10 +417,8 @@ public class AnalyticExpr extends Expr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     fnCall_.analyze(analyzer);
-    super.analyze(analyzer);
     type_ = getFnCall().getType();
 
     for (Expr e: partitionExprs_) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java b/fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
index d37152a..5e59e0f 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AnalyticInfo.java
@@ -112,7 +112,7 @@ public class AnalyticInfo extends AggregateInfoBase {
   private List<Expr> computeCommonPartitionExprs() {
     List<Expr> result = Lists.newArrayList();
     for (Expr analyticExpr: analyticExprs_) {
-      Preconditions.checkState(analyticExpr.isAnalyzed_);
+      Preconditions.checkState(analyticExpr.isAnalyzed());
       List<Expr> partitionExprs = ((AnalyticExpr) analyticExpr).getPartitionExprs();
       if (partitionExprs == null) continue;
       if (result.isEmpty()) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index ca09b05..34cf565 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -2179,7 +2179,7 @@ public class Analyzer {
   public void materializeSlots(List<Expr> exprs) {
     List<SlotId> slotIds = Lists.newArrayList();
     for (Expr e: exprs) {
-      Preconditions.checkState(e.isAnalyzed_);
+      Preconditions.checkState(e.isAnalyzed());
       e.getIds(null, slotIds);
     }
     globalState_.descTbl.markSlotsMaterialized(slotIds);
@@ -2187,7 +2187,7 @@ public class Analyzer {
 
   public void materializeSlots(Expr e) {
     List<SlotId> slotIds = Lists.newArrayList();
-    Preconditions.checkState(e.isAnalyzed_);
+    Preconditions.checkState(e.isAnalyzed());
     e.getIds(null, slotIds);
     globalState_.descTbl.markSlotsMaterialized(slotIds);
   }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
index e41cbb8..3e574b2 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
@@ -169,9 +169,7 @@ public class ArithmeticExpr extends Expr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     for (Expr child: children_) {
       Expr operand = (Expr) child;
       if (!operand.type_.isNumericType() && !operand.type_.isNull()) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java b/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
index ef19a52..8806990 100644
--- a/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
@@ -47,16 +47,14 @@ public class BetweenPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     if (children_.get(0) instanceof Subquery &&
         (children_.get(1) instanceof Subquery || children_.get(2) instanceof Subquery)) {
       throw new AnalysisException("Comparison between subqueries is not " +
           "supported in a BETWEEN predicate: " + toSqlImpl());
     }
     analyzer.castAllToCompatibleType(children_);
-    isAnalyzed_ = true;
   }
 
   public boolean isNotBetween() { return isNotBetween_; }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java b/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
index 89e3cb9..a8669e2 100644
--- a/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
@@ -168,10 +168,8 @@ public class BinaryPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     convertNumericLiteralsFromDecimal(analyzer);
     String opName = op_.getName().equals("null_matching_eq") ? "eq" : op_.getName();
     fn_ = getBuiltinFunction(analyzer, opName, collectChildReturnTypes(),

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java b/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
index e30f705..dae8bcb 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
@@ -229,10 +229,7 @@ public class CaseExpr extends Expr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     if (isDecode()) {
       Preconditions.checkState(!hasCaseExpr_);
       // decodeExpr_.analyze() would fail validating function existence. The complex

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CastExpr.java b/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
index 66ddd20..3619c46 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
@@ -70,7 +70,7 @@ public class CastExpr extends Expr {
       Preconditions.checkState(false,
           "Implicit casts should never throw analysis exception.");
     }
-    isAnalyzed_ = true;
+    analysisDone();
   }
 
   /**
@@ -198,10 +198,8 @@ public class CastExpr extends Expr {
   public boolean isImplicit() { return isImplicit_; }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     Preconditions.checkState(!isImplicit_);
-    super.analyze(analyzer);
     targetTypeDef_.analyze(analyzer);
     type_ = targetTypeDef_.getType();
     analyze();

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java b/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
index 6757d26..2655eaa 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
@@ -118,10 +118,8 @@ public class CompoundPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     // Check that children are predicates.
     for (Expr e: children_) {
       if (!e.getType().isBoolean() && !e.getType().isNull()) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java b/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
index 578c786..c8ecfc9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
@@ -57,12 +57,6 @@ public class ExistsPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-  }
-
-  @Override
   protected void toThrift(TExprNode msg) {
     // Cannot serialize a nested predicate
     Preconditions.checkState(false);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/Expr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Expr.java b/fe/src/main/java/org/apache/impala/analysis/Expr.java
index 87a2a12..69e7790 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Expr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Expr.java
@@ -176,7 +176,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
   private boolean isAuxExpr_ = false;
 
   protected Type type_;  // result of analysis
-  protected boolean isAnalyzed_;  // true after analyze() has been called
+
   protected boolean isOnClauseConjunct_; // set by analyzer
 
   // Flag to indicate whether to wrap this expr's toSql() in parenthesis. Set by parser.
@@ -198,10 +198,17 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
   // set during analysis
   protected long numDistinctValues_;
 
+  // Cached value of IsConstant(), set during analyze() and valid if isAnalyzed_ is true.
+  private boolean isConstant_;
+
   // The function to call. This can either be a scalar or aggregate function.
   // Set in analyze().
   protected Function fn_;
 
+  // True after analysis successfully completed. Protected by accessors isAnalyzed() and
+  // analysisDone().
+  private boolean isAnalyzed_ = false;
+
   protected Expr() {
     super();
     type_ = Type.INVALID;
@@ -223,6 +230,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
     selectivity_ = other.selectivity_;
     evalCost_ = other.evalCost_;
     numDistinctValues_ = other.numDistinctValues_;
+    isConstant_ = other.isConstant_;
     fn_ = other.fn_;
     children_ = Expr.cloneList(other.children_);
   }
@@ -253,7 +261,9 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
    * Throws exception if any errors found.
    * @see org.apache.impala.parser.ParseNode#analyze(org.apache.impala.parser.Analyzer)
    */
-  public void analyze(Analyzer analyzer) throws AnalysisException {
+  public final void analyze(Analyzer analyzer) throws AnalysisException {
+    if (isAnalyzed()) return;
+
     // Check the expr child limit.
     if (children_.size() > EXPR_CHILDREN_LIMIT) {
       String sql = toSql();
@@ -275,13 +285,20 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
     for (Expr child: children_) {
       child.analyze(analyzer);
     }
-    isAnalyzed_ = true;
+    if (analyzer != null) analyzer.decrementCallDepth();
     computeNumDistinctValues();
 
-    if (analyzer != null) analyzer.decrementCallDepth();
+    // Do all the analysis for the expr subclass before marking the Expr analyzed.
+    analyzeImpl(analyzer);
+    analysisDone();
   }
 
   /**
+   * Does subclass-specific analysis. Subclasses should override analyzeImpl().
+   */
+  abstract protected void analyzeImpl(Analyzer analyzer) throws AnalysisException;
+
+  /**
    * Helper function to analyze this expr and assert that the analysis was successful.
    * TODO: This function could be used in many more places to clean up. Consider
    * adding an IAnalyzable interface or similar to and move this helper into Analyzer
@@ -521,6 +538,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
     Preconditions.checkState(!type_.isNull(), "Expr has type null!");
     TExprNode msg = new TExprNode();
     msg.type = type_.toThrift();
+    msg.is_constant = isConstant_;
     msg.num_children = children_.size();
     if (fn_ != null) {
       msg.setFn(fn_.toThrift());
@@ -802,6 +820,17 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
   }
 
   /**
+   * Set the expr to be analyzed and computes isConstant_.
+   */
+  protected void analysisDone() {
+    Preconditions.checkState(!isAnalyzed_);
+    // We need to compute the const-ness as the last step, since analysis may change
+    // the result, e.g. by resolving function.
+    isConstant_ = isConstantImpl();
+    isAnalyzed_ = true;
+  }
+
+  /**
    * Resets the internal state of this expr produced by analyze().
    * Only modifies this expr, and not its child exprs.
    */
@@ -946,12 +975,29 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
   }
 
   /**
-   * @return true if this expr can be evaluated with Expr::GetValue(NULL),
-   * i.e. if it doesn't contain any references to runtime variables (e.g. slot refs).
-   * Expr subclasses should override this if necessary (e.g. SlotRef, Subquery, etc.
-   * always return false).
+   * Returns true if this expression should be treated as constant. I.e. if the frontend
+   * and backend should assume that two evaluations of the expression within a query will
+   * return the same value. Examples of constant expressions include:
+   * - Literal values like 1, "foo", or NULL
+   * - Deterministic operators applied to constant arguments, e.g. 1 + 2, or
+   *   concat("foo", "bar")
+   * - Functions that should be always return the same value within a query but may
+   *   return different values for different queries. E.g. now(), which we want to
+   *   evaluate only once during planning.
+   * May incorrectly return true if the expression is not analyzed.
+   * TODO: isAnalyzed_ should be a precondition for isConstant(), since it is not always
+   * possible to correctly determine const-ness before analysis (e.g. see
+   * FunctionCallExpr.isConstant()).
+   */
+  public final boolean isConstant() {
+    if (isAnalyzed_) return isConstant_;
+    return isConstantImpl();
+  }
+
+  /**
+   * Implements isConstant() - computes the value without using 'isConstant_'.
    */
-  public boolean isConstant() {
+  protected boolean isConstantImpl() {
     for (Expr expr : children_) {
       if (!expr.isConstant()) return false;
     }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java b/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
index a3ce06f..2355294 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
@@ -54,7 +54,7 @@ public final class ExprSubstitutionMap {
    * across query blocks. It is not required that the lhsExpr is analyzed.
    */
   public void put(Expr lhsExpr, Expr rhsExpr) {
-    Preconditions.checkState(rhsExpr.isAnalyzed_, "Rhs expr must be analyzed.");
+    Preconditions.checkState(rhsExpr.isAnalyzed(), "Rhs expr must be analyzed.");
     lhs_.add(lhsExpr);
     rhs_.add(rhsExpr);
   }
@@ -162,7 +162,7 @@ public final class ExprSubstitutionMap {
           Preconditions.checkState(false);
         }
       }
-      Preconditions.checkState(rhs_.get(i).isAnalyzed_);
+      Preconditions.checkState(rhs_.get(i).isAnalyzed());
     }
   }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/ExtractFromExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ExtractFromExpr.java b/fe/src/main/java/org/apache/impala/analysis/ExtractFromExpr.java
index f49535c..5e43f9f 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ExtractFromExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ExtractFromExpr.java
@@ -64,7 +64,9 @@ public class ExtractFromExpr extends FunctionCallExpr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    // Do these sanity checks before calling the super class to get the expected error
+    // messages if extract() is used in an invalid way.
     getFnName().analyze(analyzer);
     if (!getFnName().getFunction().equals("extract")) {
       throw new AnalysisException("Function " + getFnName().getFunction().toUpperCase()
@@ -75,8 +77,8 @@ public class ExtractFromExpr extends FunctionCallExpr {
       throw new AnalysisException("Function " + getFnName().toString() + " conflicts " +
           "with the EXTRACT builtin.");
     }
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+
+    super.analyzeImpl(analyzer);
 
     String extractFieldIdent = ((StringLiteral)children_.get(1)).getValue();
     Preconditions.checkNotNull(extractFieldIdent);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
index 3452b05..15af25f 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
@@ -96,7 +96,7 @@ public class FunctionCallExpr extends Expr {
    */
   public static FunctionCallExpr createMergeAggCall(
       FunctionCallExpr agg, List<Expr> params) {
-    Preconditions.checkState(agg.isAnalyzed_);
+    Preconditions.checkState(agg.isAnalyzed());
     Preconditions.checkState(agg.isAggregateFunction());
     FunctionCallExpr result = new FunctionCallExpr(
         agg.fnName_, new FunctionParams(false, params), true);
@@ -139,7 +139,7 @@ public class FunctionCallExpr extends Expr {
 
   @Override
   public void resetAnalysisState() {
-    isAnalyzed_ = false;
+    super.resetAnalysisState();
     // Resolving merge agg functions after substitution may fail e.g., if the
     // intermediate agg type is not the same as the output type. Preserve the original
     // fn_ such that analyze() hits the special-case code for merge agg fns that
@@ -232,14 +232,13 @@ public class FunctionCallExpr extends Expr {
     }
   }
 
-  /**
-   * Needs to be kept in sync with the BE understanding of constness in
-   * scalar-fn-call.h/cc.
-   */
   @Override
-  public boolean isConstant() {
+  protected boolean isConstantImpl() {
+    // TODO: we can't correctly determine const-ness before analyzing 'fn_'. We should
+    // rework logic so that we do not call this function on unanalyzed exprs.
     // Aggregate functions are never constant.
     if (fn_ instanceof AggregateFunction) return false;
+
     String fnName = fnName_.getFunction();
     if (fnName == null) {
       // This expr has not been analyzed yet, get the function name from the path.
@@ -253,7 +252,7 @@ public class FunctionCallExpr extends Expr {
     }
     // Sleep is a special function for testing.
     if (fnName.equalsIgnoreCase("sleep")) return false;
-    return super.isConstant();
+    return super.isConstantImpl();
   }
 
   // Provide better error message for some aggregate builtins. These can be
@@ -381,9 +380,7 @@ public class FunctionCallExpr extends Expr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     fnName_.analyze(analyzer);
 
     if (isMergeAggFn_) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/InPredicate.java b/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
index 8ce4497..d95fb3a 100644
--- a/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
@@ -104,10 +104,8 @@ public class InPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     if (contains(Subquery.class)) {
       // An [NOT] IN predicate with a subquery must contain two children, the second of
       // which is a Subquery.

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java b/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java
index 20c73b7..380fcf4 100644
--- a/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java
@@ -40,9 +40,8 @@ public class IsNotEmptyPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     if (!getChild(0).getType().isCollectionType()) {
       throw new AnalysisException("Operand must be a collection type: "
           + getChild(0).toSql() + " is of type " + getChild(0).getType());

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java b/fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
index 9092a32..fb3e964 100644
--- a/fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
@@ -97,10 +97,8 @@ public class IsNullPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     if (contains(Subquery.class)) {
       if (getChild(0) instanceof ExistsPredicate) {
         // Replace the EXISTS subquery with a BoolLiteral as it can never return

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java b/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
index 8a60af4..9fab11e 100644
--- a/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
@@ -114,9 +114,8 @@ public class LikePredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     if (!getChild(0).getType().isStringType() && !getChild(0).getType().isNull()) {
       throw new AnalysisException(
           "left operand of " + op_.toString() + " must be of type STRING: " + toSql());

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/LimitElement.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/LimitElement.java b/fe/src/main/java/org/apache/impala/analysis/LimitElement.java
index 4645b95..a8ef9e2 100644
--- a/fe/src/main/java/org/apache/impala/analysis/LimitElement.java
+++ b/fe/src/main/java/org/apache/impala/analysis/LimitElement.java
@@ -107,43 +107,37 @@ class LimitElement {
   public void analyze(Analyzer analyzer) throws AnalysisException {
     isAnalyzed_ = true;
     if (limitExpr_ != null) {
-      if (!limitExpr_.isConstant()) {
-        throw new AnalysisException("LIMIT expression must be a constant expression: " +
-            limitExpr_.toSql());
-      }
-
-      limitExpr_.analyze(analyzer);
-      if (!limitExpr_.getType().isIntegerType()) {
-        throw new AnalysisException("LIMIT expression must be an integer type but is '" +
-            limitExpr_.getType() + "': " + limitExpr_.toSql());
-      }
       limit_ = evalIntegerExpr(analyzer, limitExpr_, "LIMIT");
     }
     if (limit_ == 0) analyzer.setHasEmptyResultSet();
-
     if (offsetExpr_ != null) {
-      if (!offsetExpr_.isConstant()) {
-        throw new AnalysisException("OFFSET expression must be a constant expression: " +
-            offsetExpr_.toSql());
-      }
-
-      offsetExpr_.analyze(analyzer);
-      if (!offsetExpr_.getType().isIntegerType()) {
-        throw new AnalysisException("OFFSET expression must be an integer type but " +
-            "is '" + offsetExpr_.getType() + "': " + offsetExpr_.toSql());
-      }
       offset_ = evalIntegerExpr(analyzer, offsetExpr_, "OFFSET");
     }
   }
 
   /**
-   * Evaluations an expression to a non-zero integral value, returned as a long. Throws
-   * if the expression cannot be evaluated, if the value evaluates to null, or if the
-   * result is negative. The 'name' parameter is used in exception messages, e.g.
+   * Analyzes and evaluates expression to a non-zero integral value, returned as a long.
+   * Throws if the expression cannot be evaluated, if the value evaluates to null, or if
+   * the result is negative. The 'name' parameter is used in exception messages, e.g.
    * "LIMIT expression evaluates to NULL".
    */
   private static long evalIntegerExpr(Analyzer analyzer, Expr expr, String name)
       throws AnalysisException {
+    // Check for slotrefs before analysis so we can provide a more helpful message than
+    // "Could not resolve column/field reference".
+    if (expr.contains(SlotRef.class)) {
+      throw new AnalysisException(name + " expression must be a constant expression: " +
+          expr.toSql());
+    }
+    expr.analyze(analyzer);
+    if (!expr.isConstant()) {
+      throw new AnalysisException(name + " expression must be a constant expression: " +
+          expr.toSql());
+    }
+    if (!expr.getType().isIntegerType()) {
+      throw new AnalysisException(name + " expression must be an integer type but is '" +
+          expr.getType() + "': " + expr.toSql());
+    }
     TColumnValue val = null;
     try {
       val = FeSupport.EvalExprWithoutRow(expr, analyzer.getQueryCtx());

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java b/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
index db0b442..45a65f9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
@@ -96,6 +96,11 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr
     return (LiteralExpr) e.uncheckedCastTo(type);
   }
 
+  @Override
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    // Literals require no analysis.
+  }
+
   /**
    * Returns an analyzed literal from the thrift object.
    */

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java b/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
index 8cf102a..79c906c 100644
--- a/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
+++ b/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
@@ -56,7 +56,7 @@ public class NumericLiteral extends LiteralExpr {
   private boolean explicitlyCast_;
 
   public NumericLiteral(BigDecimal value) {
-    init(value);
+    value_ = value;
   }
 
   public NumericLiteral(String value, Type t) throws AnalysisException {
@@ -66,7 +66,7 @@ public class NumericLiteral extends LiteralExpr {
     } catch (NumberFormatException e) {
       throw new AnalysisException("invalid numeric literal: " + value, e);
     }
-    init(val);
+    value_ = val;
     this.analyze(null);
     if (type_.isDecimal() && t.isDecimal()) {
       // Verify that the input decimal value is consistent with the specified
@@ -88,19 +88,19 @@ public class NumericLiteral extends LiteralExpr {
    * type is preserved across substitutions and re-analysis.
    */
   public NumericLiteral(BigInteger value, Type type) {
-    isAnalyzed_ = true;
     value_ = new BigDecimal(value);
     type_ = type;
     evalCost_ = LITERAL_COST;
     explicitlyCast_ = true;
+    analysisDone();
   }
 
   public NumericLiteral(BigDecimal value, Type type) {
-    isAnalyzed_ = true;
     value_ = value;
     type_ = type;
     evalCost_ = LITERAL_COST;
     explicitlyCast_ = true;
+    analysisDone();
   }
 
   /**
@@ -178,9 +178,7 @@ public class NumericLiteral extends LiteralExpr {
   public BigDecimal getValue() { return value_; }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     if (!explicitlyCast_) {
       // Compute the precision and scale from the BigDecimal.
       type_ = TypesUtil.computeDecimalType(value_);
@@ -225,7 +223,6 @@ public class NumericLiteral extends LiteralExpr {
       }
     }
     evalCost_ = LITERAL_COST;
-    isAnalyzed_ = true;
   }
 
   /**
@@ -251,7 +248,7 @@ public class NumericLiteral extends LiteralExpr {
     if (targetType.isDecimal()) {
       ScalarType decimalType = (ScalarType) targetType;
       // analyze() ensures that value_ never exceeds the maximum scale and precision.
-      Preconditions.checkState(isAnalyzed_);
+      Preconditions.checkState(isAnalyzed());
       // Sanity check that our implicit casting does not allow a reduced precision or
       // truncating values from the right of the decimal point.
       Preconditions.checkState(value_.precision() <= decimalType.decimalPrecision());
@@ -278,11 +275,6 @@ public class NumericLiteral extends LiteralExpr {
     return value_.compareTo(other.value_);
   }
 
-  private void init(BigDecimal value) {
-    isAnalyzed_ = false;
-    value_ = value;
-  }
-
   // Returns the unscaled value of this literal. BigDecimal doesn't treat scale
   // the way we do. We need to pad it out with zeros or truncate as necessary.
   private BigInteger getUnscaledValue() {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/Predicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Predicate.java b/fe/src/main/java/org/apache/impala/analysis/Predicate.java
index eacf0c5..d1311b7 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Predicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Predicate.java
@@ -42,9 +42,7 @@ public abstract class Predicate extends Expr {
   public void setIsEqJoinConjunct(boolean v) { isEqJoinConjunct_ = v; }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     type_ = Type.BOOLEAN;
     // values: true/false/null
     numDistinctValues_ = 3;

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/SlotRef.java b/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
index 1d1e319..5f77f3b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
@@ -71,13 +71,13 @@ public class SlotRef extends Expr {
     } else {
       rawPath_ = null;
     }
-    isAnalyzed_ = true;
     desc_ = desc;
     type_ = desc.getType();
     evalCost_ = SLOT_REF_COST;
     String alias = desc.getParent().getAlias();
     label_ = (alias != null ? alias + "." : "") + desc.getLabel();
     numDistinctValues_ = desc.getStats().getNumDistinctValues();
+    analysisDone();
   }
 
   /**
@@ -89,13 +89,10 @@ public class SlotRef extends Expr {
     label_ = other.label_;
     desc_ = other.desc_;
     type_ = other.type_;
-    isAnalyzed_ = other.isAnalyzed_;
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     Path resolvedPath = null;
     try {
       resolvedPath = analyzer.resolvePath(rawPath_, PathType.SLOT_REF);
@@ -124,26 +121,25 @@ public class SlotRef extends Expr {
       // The NDV cannot exceed the #rows in the table.
       numDistinctValues_ = Math.min(numDistinctValues_, rootTable.getNumRows());
     }
-    isAnalyzed_ = true;
   }
 
   @Override
-  public boolean isConstant() { return false; }
+  protected boolean isConstantImpl() { return false; }
 
   public SlotDescriptor getDesc() {
-    Preconditions.checkState(isAnalyzed_);
+    Preconditions.checkState(isAnalyzed());
     Preconditions.checkNotNull(desc_);
     return desc_;
   }
 
   public SlotId getSlotId() {
-    Preconditions.checkState(isAnalyzed_);
+    Preconditions.checkState(isAnalyzed());
     Preconditions.checkNotNull(desc_);
     return desc_.getId();
   }
 
   public Path getResolvedPath() {
-    Preconditions.checkState(isAnalyzed_);
+    Preconditions.checkState(isAnalyzed());
     return desc_.getPath();
   }
 
@@ -208,7 +204,7 @@ public class SlotRef extends Expr {
 
   @Override
   public boolean isBoundBySlotIds(List<SlotId> slotIds) {
-    Preconditions.checkState(isAnalyzed_);
+    Preconditions.checkState(isAnalyzed());
     return slotIds.contains(desc_.getId());
   }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/Subquery.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Subquery.java b/fe/src/main/java/org/apache/impala/analysis/Subquery.java
index afb839e..ab3132c 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Subquery.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Subquery.java
@@ -72,9 +72,7 @@ public class Subquery extends Expr {
    * Analyzes the subquery in a child analyzer.
    */
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     if (!(stmt_ instanceof SelectStmt)) {
       throw new AnalysisException("A subquery must contain a single select block: " +
         toSql());
@@ -100,11 +98,10 @@ public class Subquery extends Expr {
     if (!((SelectStmt)stmt_).returnsSingleRow()) type_ = new ArrayType(type_);
 
     Preconditions.checkNotNull(type_);
-    isAnalyzed_ = true;
   }
 
   @Override
-  public boolean isConstant() { return false; }
+  protected boolean isConstantImpl() { return false; }
 
   /**
    * Check if the subquery's SelectStmt returns a single column of scalar type.

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java b/fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
index 8faeb27..104cd1a 100644
--- a/fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
@@ -116,10 +116,7 @@ public class TimestampArithmeticExpr extends Expr {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
-
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     if (funcName_ != null) {
       // Set op based on funcName for function-call like version.
       if (funcName_.equals("date_add")) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java b/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
index 49ebc4e..f55ed81 100644
--- a/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
@@ -57,9 +57,8 @@ public class TupleIsNullPredicate extends Predicate {
   }
 
   @Override
-  public void analyze(Analyzer analyzer) throws AnalysisException {
-    if (isAnalyzed_) return;
-    super.analyze(analyzer);
+  protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
+    super.analyzeImpl(analyzer);
     analyzer_ = analyzer;
     evalCost_ = tupleIds_.size() * IS_NULL_COST;
   }
@@ -98,7 +97,7 @@ public class TupleIsNullPredicate extends Predicate {
   }
 
   @Override
-  public boolean isConstant() { return false; }
+  protected boolean isConstantImpl() { return false; }
 
   /**
    * Makes each input expr nullable, if necessary, by wrapping it as follows:

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a4eb4705/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
index ff99085..aab2dba 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
@@ -494,6 +494,11 @@ public class AnalyzerTest extends FrontendTestBase {
     AnalysisError("select id, bool_col from functional.AllTypes order by id limit 10 " +
         "offset id < 10",
         "OFFSET expression must be a constant expression: id < 10");
+    AnalysisError("select id, bool_col from functional.AllTypes limit count(*)",
+        "LIMIT expression must be a constant expression: count(*)");
+    AnalysisError("select id, bool_col from functional.AllTypes order by id limit 10 " +
+        "offset count(*)",
+        "OFFSET expression must be a constant expression: count(*)");
 
     // Offset is only valid with an order by
     AnalysisError("SELECT a FROM test LIMIT 10 OFFSET 5",


Mime
View raw message