You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/01/31 18:09:31 UTC
incubator-impala git commit: IMPALA-4617: remove IsConstant()
analysis from be
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 <ta...@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 <ta...@cloudera.com>
Authored: Fri Dec 16 07:41:43 2016 -0800
Committer: Impala Public Jenkins <im...@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",