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 <[email protected]> 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 <[email protected]> Authored: Fri Dec 16 07:41:43 2016 -0800 Committer: Impala Public Jenkins <[email protected]> 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",
