IMPALA-1788: Fold constant expressions. Adds a new ExprRewriteRule for replacing constant expressions with their literal equivalent via BE evaluation. Applies the new rule together with the existing ones on the parse tree, after analysis.
Limitations - Constant folding is applied on the unresolved expressions. As a result, it only works for expressions that are constant within a single query block, as opposed to expressions that may become constant after fully substituting inline-view exprs. - Exprs are not normalized, so some opportunities for constant folding are missed for certain expr-tree shapes. This patch includes the following interesting changes: - Introduces a timestamp literal that can only be produced by constant folding (not expressible directly via SQL). - To make sure that rewrites have no user-visible effect, the original result types and column labels of the top-level statement are restored after the rewrites are performed. - Does not fold exprs if their evaluation resulted in a warning or error, or if the resulting value is not representable by corresponding FE LiteralExpr. - Fixes an existing issue with converting strings between the FE/BE. String produced in the BE that have characters with a value > 127 are not correctly deserialized into a Java String via thrift. We detect this case during constant folding and abandon folding of such exprs. - Fixes several issues with detecting/reporting errors in NativeEvalConstExprs(). - Cleans up ExprContext::GetValue() into ExprContext::GetConstantValue() which clarifies its only use of evaluating exprs from the FE. Testing: - Modifies expr-test.cc to run all tests through the constant folding path. - Adds basic planner and rewrite rule tests. - Exhaustive test run passed Change-Id: If672b703db1ba0bfc26e5b9130161798b40a69e9 Reviewed-on: http://gerrit.cloudera.org:8080/5109 Reviewed-by: Alex Behm <[email protected]> Tested-by: Internal 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/bbf5255d Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/bbf5255d Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/bbf5255d Branch: refs/heads/master Commit: bbf5255d0e829ab553b57052ff9383aa365c7d59 Parents: 1e30621 Author: Alex Behm <[email protected]> Authored: Thu Nov 3 20:53:58 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Wed Nov 23 21:11:30 2016 +0000 ---------------------------------------------------------------------- be/src/exprs/expr-context.cc | 24 +-- be/src/exprs/expr-context.h | 21 +- be/src/exprs/expr-test.cc | 33 ++- be/src/exprs/expr.cc | 1 + be/src/exprs/literal.cc | 47 ++++- be/src/exprs/literal.h | 2 + be/src/runtime/runtime-state.h | 6 + be/src/service/fe-support.cc | 73 ++++--- common/thrift/Exprs.thrift | 18 +- .../apache/impala/analysis/AnalysisContext.java | 18 ++ .../org/apache/impala/analysis/Analyzer.java | 6 + .../apache/impala/analysis/BinaryPredicate.java | 5 +- .../org/apache/impala/analysis/BoolLiteral.java | 1 + .../analysis/CreateTableAsSelectStmt.java | 15 +- .../java/org/apache/impala/analysis/Expr.java | 31 +-- .../impala/analysis/FunctionCallExpr.java | 19 +- .../org/apache/impala/analysis/InPredicate.java | 4 +- .../org/apache/impala/analysis/InsertStmt.java | 8 +- .../org/apache/impala/analysis/LiteralExpr.java | 40 +++- .../org/apache/impala/analysis/ModifyStmt.java | 9 + .../org/apache/impala/analysis/NullLiteral.java | 1 + .../apache/impala/analysis/PartitionSet.java | 3 +- .../org/apache/impala/analysis/QueryStmt.java | 11 +- .../org/apache/impala/analysis/SelectStmt.java | 7 +- .../org/apache/impala/analysis/SlotRef.java | 6 +- .../apache/impala/analysis/StatementBase.java | 46 ++++ .../apache/impala/analysis/StringLiteral.java | 29 +-- .../org/apache/impala/analysis/Subquery.java | 9 +- .../impala/analysis/TimestampLiteral.java | 105 ++++++++++ .../org/apache/impala/analysis/UnionStmt.java | 2 +- .../apache/impala/planner/HBaseScanNode.java | 37 ++-- .../impala/planner/HdfsPartitionPruner.java | 5 +- .../org/apache/impala/planner/KuduScanNode.java | 13 +- .../java/org/apache/impala/planner/Planner.java | 5 - .../impala/planner/SingleNodePlanner.java | 6 +- .../apache/impala/rewrite/ExprRewriteRule.java | 5 + .../org/apache/impala/rewrite/ExprRewriter.java | 2 +- .../impala/rewrite/FoldConstantsRule.java | 63 ++++++ .../impala/analysis/AnalyzeExprsTest.java | 4 +- .../impala/analysis/ExprRewriteRulesTest.java | 33 ++- .../org/apache/impala/planner/PlannerTest.java | 8 + .../org/apache/impala/testutil/TestUtils.java | 13 +- .../queries/PlannerTest/aggregation.test | 22 +- .../queries/PlannerTest/conjunct-ordering.test | 4 +- .../queries/PlannerTest/constant-folding.test | 209 +++++++++++++++++++ .../queries/PlannerTest/empty.test | 6 +- .../queries/PlannerTest/insert.test | 2 +- .../queries/PlannerTest/join-order.test | 16 +- .../queries/PlannerTest/joins.test | 4 +- .../queries/PlannerTest/kudu.test | 6 +- .../queries/PlannerTest/outer-joins.test | 2 +- .../PlannerTest/predicate-propagation.test | 6 +- .../queries/PlannerTest/subquery-rewrite.test | 2 +- .../queries/PlannerTest/tpcds-all.test | 98 ++++----- .../queries/PlannerTest/values.test | 8 +- .../queries/QueryTest/exprs.test | 32 +++ .../functional-query/queries/QueryTest/udf.test | 30 +-- tests/query_test/test_exprs.py | 8 +- 58 files changed, 949 insertions(+), 300 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/expr-context.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr-context.cc b/be/src/exprs/expr-context.cc index f97a55d..27c61c1 100644 --- a/be/src/exprs/expr-context.cc +++ b/be/src/exprs/expr-context.cc @@ -150,13 +150,9 @@ void ExprContext::FreeLocalAllocations(const vector<FunctionContext*>& fn_ctxs) } } -void ExprContext::GetValue(const TupleRow* row, bool as_ascii, TColumnValue* col_val) { - void* value = GetValue(row); - if (as_ascii) { - RawValue::PrintValue(value, root_->type_, root_->output_scale_, &col_val->string_val); - col_val->__isset.string_val = true; - return; - } +void ExprContext::GetConstantValue(TColumnValue* col_val) { + DCHECK(root_->IsConstant()); + void* value = GetValue(NULL); if (value == NULL) return; StringValue* string_val = NULL; @@ -206,19 +202,23 @@ void ExprContext::GetValue(const TupleRow* row, bool as_ascii, TColumnValue* col case TYPE_VARCHAR: string_val = reinterpret_cast<StringValue*>(value); tmp.assign(static_cast<char*>(string_val->ptr), string_val->len); - col_val->string_val.swap(tmp); - col_val->__isset.string_val = true; + col_val->binary_val.swap(tmp); + col_val->__isset.binary_val = true; break; case TYPE_CHAR: tmp.assign(StringValue::CharSlotToPtr(value, root_->type_), root_->type_.len); - col_val->string_val.swap(tmp); - col_val->__isset.string_val = true; + col_val->binary_val.swap(tmp); + col_val->__isset.binary_val = true; break; - case TYPE_TIMESTAMP: + case TYPE_TIMESTAMP: { + uint8_t* uint8_val = reinterpret_cast<uint8_t*>(value); + col_val->binary_val.assign(uint8_val, uint8_val + root_->type_.GetSlotSize()); + col_val->__isset.binary_val = true; RawValue::PrintValue( value, root_->type_, root_->output_scale_, &col_val->string_val); col_val->__isset.string_val = true; break; + } default: DCHECK(false) << "bad GetValue() type: " << root_->type_.DebugString(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/expr-context.h ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr-context.h b/be/src/exprs/expr-context.h index 39abc5f..e765b87 100644 --- a/be/src/exprs/expr-context.h +++ b/be/src/exprs/expr-context.h @@ -74,22 +74,19 @@ class ExprContext { /// result in result_. void* GetValue(const TupleRow* row); - /// Convenience function: extract value into col_val and sets the - /// appropriate __isset flag. - /// If the value is NULL and as_ascii is false, nothing is set. - /// If 'as_ascii' is true, writes the value in ascii into stringVal - /// (nulls turn into "NULL"); - /// if it is false, the specific field in col_val that receives the value is - /// based on the type of the expr: + /// Convenience function for evaluating constant Exprs from the FE. Extracts value into + /// col_val and sets the appropriate __isset flag. No fields are set for NULL values. + /// The specific field in col_val that receives the value is based on the expr type: /// TYPE_BOOLEAN: boolVal /// TYPE_TINYINT/SMALLINT/INT: intVal /// TYPE_BIGINT: longVal /// TYPE_FLOAT/DOUBLE: doubleVal - /// TYPE_STRING: stringVal - /// TYPE_TIMESTAMP: stringVal - /// Note: timestamp is converted to string via RawValue::PrintValue because HiveServer2 - /// requires timestamp in a string format. - void GetValue(const TupleRow* row, bool as_ascii, TColumnValue* col_val); + /// TYPE_STRING: binaryVal. Do not populate stringVal directly because BE/FE + /// conversions do not work properly for strings with ASCII chars + /// above 127. Pass the raw bytes so the caller can decide what to + /// do with the result (e.g., bail constant folding). + /// TYPE_TIMESTAMP: binaryVal has the raw data, stringVal its string representation. + void GetConstantValue(TColumnValue* col_val); /// Convenience functions: print value into 'str' or 'stream'. NULL turns into "NULL". void PrintValue(const TupleRow* row, std::string* str); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/expr-test.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc index 5a999b6..2a4e4ff 100644 --- a/be/src/exprs/expr-test.cc +++ b/be/src/exprs/expr-test.cc @@ -73,6 +73,7 @@ using namespace llvm; namespace impala { ImpaladQueryExecutor* executor_; bool disable_codegen_; +bool enable_expr_rewrites_; template <typename ORIGINAL_TYPE, typename VAL_TYPE> string LiteralToString(VAL_TYPE val) { @@ -972,7 +973,7 @@ bool ExprTest::ConvertValue<bool>(const string& value) { if (value.compare("false") == 0) { return false; } else { - DCHECK(value.compare("true") == 0); + DCHECK(value.compare("true") == 0) << value; return true; } } @@ -5048,7 +5049,11 @@ TEST_F(ExprTest, ResultsLayoutTest) { expected_offsets.clear(); // With one expr, all offsets should be 0. expected_offsets[t.GetByteSize()] = set<int>({0}); - exprs.push_back(pool.Add(Literal::CreateLiteral(t, "0"))); + if (t.type != TYPE_TIMESTAMP) { + exprs.push_back(pool.Add(Literal::CreateLiteral(t, "0"))); + } else { + exprs.push_back(pool.Add(Literal::CreateLiteral(t, "2016-11-09"))); + } if (t.IsVarLenStringType()) { ValidateLayout(exprs, 16, 0, expected_offsets); } else { @@ -5106,8 +5111,8 @@ TEST_F(ExprTest, ResultsLayoutTest) { expected_byte_size += 5 * 8; // No more padding ASSERT_EQ(expected_byte_size % 8, 0); - exprs.push_back(pool.Add(Literal::CreateLiteral(TYPE_TIMESTAMP, "0"))); - exprs.push_back(pool.Add(Literal::CreateLiteral(TYPE_TIMESTAMP, "0"))); + exprs.push_back(pool.Add(Literal::CreateLiteral(TYPE_TIMESTAMP, "2016-11-09"))); + exprs.push_back(pool.Add(Literal::CreateLiteral(TYPE_TIMESTAMP, "2016-11-09"))); exprs.push_back(pool.Add( Literal::CreateLiteral(ColumnType::CreateDecimalType(20, 0), "0"))); expected_offsets[16].insert(expected_byte_size); @@ -6123,21 +6128,35 @@ int main(int argc, char **argv) { impala_server->beeswax_port()); ABORT_IF_ERROR(executor_->Setup()); - vector<string> options; - // Disable FE Expr rewrites to make sure the Exprs get executed exactly as specified + // Disable FE expr rewrites to make sure the Exprs get executed exactly as specified // in the tests here. + vector<string> options; options.push_back("ENABLE_EXPR_REWRITES=0"); options.push_back("DISABLE_CODEGEN=1"); disable_codegen_ = true; + enable_expr_rewrites_ = false; executor_->setExecOptions(options); - cout << "Running without codegen" << endl; int ret = RUN_ALL_TESTS(); if (ret != 0) return ret; + options.clear(); + options.push_back("ENABLE_EXPR_REWRITES=0"); options.push_back("DISABLE_CODEGEN=0"); disable_codegen_ = false; + enable_expr_rewrites_ = false; executor_->setExecOptions(options); cout << endl << "Running with codegen" << endl; + ret = RUN_ALL_TESTS(); + if (ret != 0) return ret; + + // Enable FE expr rewrites to get test for constant folding over all exprs. + options.clear(); + options.push_back("ENABLE_EXPR_REWRITES=1"); + options.push_back("DISABLE_CODEGEN=1"); + disable_codegen_ = true; + enable_expr_rewrites_ = true; + executor_->setExecOptions(options); + cout << endl << "Running without codegen and expr rewrites" << endl; return RUN_ALL_TESTS(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/expr.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr.cc b/be/src/exprs/expr.cc index 045c030..3f2ef7b 100644 --- a/be/src/exprs/expr.cc +++ b/be/src/exprs/expr.cc @@ -205,6 +205,7 @@ Status Expr::CreateExpr(ObjectPool* pool, const TExprNode& texpr_node, Expr** ex case TExprNodeType::INT_LITERAL: case TExprNodeType::STRING_LITERAL: case TExprNodeType::DECIMAL_LITERAL: + case TExprNodeType::TIMESTAMP_LITERAL: *expr = pool->Add(new Literal(texpr_node)); return Status::OK(); case TExprNodeType::CASE_EXPR: http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/literal.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/literal.cc b/be/src/exprs/literal.cc index 5b075f6..4fd4e2c 100644 --- a/be/src/exprs/literal.cc +++ b/be/src/exprs/literal.cc @@ -18,12 +18,15 @@ #include "literal.h" #include <sstream> +#include <boost/date_time/posix_time/posix_time_types.hpp> #include "codegen/codegen-anyval.h" #include "codegen/llvm-codegen.h" #include "gen-cpp/Exprs_types.h" #include "runtime/decimal-value.inline.h" #include "runtime/runtime-state.h" +#include "runtime/timestamp-parse-util.h" +#include "gen-cpp/Exprs_types.h" #include "common/names.h" @@ -120,6 +123,14 @@ Literal::Literal(const TExprNode& node) } break; } + case TYPE_TIMESTAMP: { + DCHECK_EQ(node.node_type, TExprNodeType::TIMESTAMP_LITERAL); + DCHECK(node.__isset.timestamp_literal); + const string& ts_val = node.timestamp_literal.value; + DCHECK_EQ(type_.GetSlotSize(), ts_val.size()); + memcpy(&value_.timestamp_val, ts_val.data(), type_.GetSlotSize()); + break; + } default: DCHECK(false) << "Invalid type: " << TypeToString(type_.type); } @@ -197,6 +208,12 @@ Literal::Literal(ColumnType type, const StringValue& v) : Expr(type) { DCHECK(type.type == TYPE_STRING || type.type == TYPE_CHAR) << type; } +Literal::Literal(ColumnType type, const TimestampValue& v) + : Expr(type) { + DCHECK_EQ(type.type, TYPE_TIMESTAMP) << type; + value_.timestamp_val = v; +} + template<class T> bool ParseString(const string& str, T* val) { istringstream stream(str); @@ -204,6 +221,16 @@ bool ParseString(const string& str, T* val) { return !stream.fail(); } +template<> +bool ParseString(const string& str, TimestampValue* val) { + boost::gregorian::date date; + boost::posix_time::time_duration time; + bool success = TimestampParser::Parse(str.data(), str.length(), &date, &time); + val->set_date(date); + val->set_time(time); + return success; +} + Literal* Literal::CreateLiteral(const ColumnType& type, const string& str) { switch (type.type) { case TYPE_BOOLEAN: { @@ -246,8 +273,8 @@ Literal* Literal::CreateLiteral(const ColumnType& type, const string& str) { case TYPE_CHAR: return new Literal(type, str); case TYPE_TIMESTAMP: { - double v = 0; - DCHECK(ParseString<double>(str, &v)); + TimestampValue v; + DCHECK(ParseString<TimestampValue>(str, &v)); return new Literal(type, v); } case TYPE_DECIMAL: { @@ -319,6 +346,13 @@ DecimalVal Literal::GetDecimalVal(ExprContext* context, const TupleRow* row) { return DecimalVal(); } +TimestampVal Literal::GetTimestampVal(ExprContext* context, const TupleRow* row) { + DCHECK_EQ(type_.type, TYPE_TIMESTAMP) << type_; + TimestampVal result; + value_.timestamp_val.ToTimestampVal(&result); + return result; +} + string Literal::DebugString() const { stringstream out; out << "Literal(value="; @@ -362,6 +396,9 @@ string Literal::DebugString() const { DCHECK(false) << type_.DebugString(); } break; + case TYPE_TIMESTAMP: + out << value_.timestamp_val; + break; default: out << "[bad type! " << type_ << "]"; } @@ -432,6 +469,12 @@ Status Literal::GetCodegendComputeFn(LlvmCodeGen* codegen, llvm::Function** fn) DCHECK(false) << type_.DebugString(); } break; + case TYPE_TIMESTAMP: + v.SetTimeOfDay(builder.getInt64( + *reinterpret_cast<const int64_t*>(&value_.timestamp_val.time()))); + v.SetDate(builder.getInt32( + *reinterpret_cast<const int32_t*>(&value_.timestamp_val.date()))); + break; default: stringstream ss; ss << "Invalid type: " << type_; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/exprs/literal.h ---------------------------------------------------------------------- diff --git a/be/src/exprs/literal.h b/be/src/exprs/literal.h index 65219e0..b6d1a49 100644 --- a/be/src/exprs/literal.h +++ b/be/src/exprs/literal.h @@ -40,6 +40,7 @@ class Literal: public Expr { Literal(ColumnType type, double v); Literal(ColumnType type, const std::string& v); Literal(ColumnType type, const StringValue& v); + Literal(ColumnType type, const TimestampValue& v); /// Test function that parses 'str' according to 'type'. The caller owns the returned /// Literal. @@ -56,6 +57,7 @@ class Literal: public Expr { virtual impala_udf::DoubleVal GetDoubleVal(ExprContext*, const TupleRow*); virtual impala_udf::StringVal GetStringVal(ExprContext*, const TupleRow*); virtual impala_udf::DecimalVal GetDecimalVal(ExprContext*, const TupleRow*); + virtual impala_udf::TimestampVal GetTimestampVal(ExprContext*, const TupleRow*); protected: friend class Expr; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/runtime/runtime-state.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/runtime-state.h b/be/src/runtime/runtime-state.h index f86374e..7003e59 100644 --- a/be/src/runtime/runtime-state.h +++ b/be/src/runtime/runtime-state.h @@ -232,6 +232,12 @@ class RuntimeState { return error_log_.size() < query_options().max_errors; } + /// Returns true if there are entries in the error log. + bool HasErrors() { + boost::lock_guard<SpinLock> l(error_log_lock_); + return !error_log_.empty(); + } + /// Returns the error log lines as a string joined with '\n'. std::string ErrorLog(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/be/src/service/fe-support.cc ---------------------------------------------------------------------- diff --git a/be/src/service/fe-support.cc b/be/src/service/fe-support.cc index 52beda5..b33cee1 100644 --- a/be/src/service/fe-support.cc +++ b/be/src/service/fe-support.cc @@ -60,7 +60,9 @@ Java_org_apache_impala_service_FeSupport_NativeFeTestInit( JNIEnv* env, jclass caller_class) { DCHECK(ExecEnv::GetInstance() == NULL) << "This should only be called once from the FE"; char* name = const_cast<char*>("FeSupport"); - InitCommonRuntime(1, &name, false, TestInfo::FE_TEST); + // Init the JVM to load the classes in JniUtil that are needed for returning + // exceptions to the FE. + InitCommonRuntime(1, &name, true, TestInfo::FE_TEST); LlvmCodeGen::InitializeLlvm(true); ExecEnv* exec_env = new ExecEnv(); // This also caches it from the process. exec_env->InitForFeTests(); @@ -69,7 +71,8 @@ Java_org_apache_impala_service_FeSupport_NativeFeTestInit( // Evaluates a batch of const exprs and returns the results in a serialized // TResultRow, where each TColumnValue in the TResultRow stores the result of // a predicate evaluation. It requires JniUtil::Init() to have been -// called. +// called. Throws a Java exception if an error or warning is encountered during +// the expr evaluation. extern "C" JNIEXPORT jbyteArray JNICALL Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs( @@ -85,24 +88,38 @@ Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs( DeserializeThriftMsg(env, thrift_expr_batch, &expr_batch); DeserializeThriftMsg(env, thrift_query_ctx_bytes, &query_ctx); vector<TExpr>& texprs = expr_batch.exprs; - // Disable codegen advisorily to avoid unnecessary latency. + // Disable codegen advisory to avoid unnecessary latency. query_ctx.disable_codegen_hint = true; + // Allow logging of at least one error, so we can detect and convert it into a + // Java exception. + query_ctx.request.query_options.max_errors = 1; RuntimeState state(query_ctx); THROW_IF_ERROR_RET(jni_frame.push(env), env, JniUtil::internal_exc_class(), result_bytes); // Exprs can allocate memory so we need to set up the mem trackers before // preparing/running the exprs. - state.InitMemTrackers(TUniqueId(), NULL, -1); + int64_t mem_limit = -1; + if (query_ctx.request.query_options.__isset.mem_limit + && query_ctx.request.query_options.mem_limit > 0) { + mem_limit = query_ctx.request.query_options.mem_limit; + } + state.InitMemTrackers(state.query_id(), NULL, mem_limit); + // Prepare() the exprs. Always Close() the exprs even in case of errors. vector<ExprContext*> expr_ctxs; for (const TExpr& texpr : texprs) { ExprContext* ctx; THROW_IF_ERROR_RET(Expr::CreateExprTree(&obj_pool, texpr, &ctx), env, JniUtil::internal_exc_class(), result_bytes); - THROW_IF_ERROR_RET(ctx->Prepare(&state, RowDescriptor(), state.query_mem_tracker()), - env, JniUtil::internal_exc_class(), result_bytes); + Status prepare_status = + ctx->Prepare(&state, RowDescriptor(), state.query_mem_tracker()); expr_ctxs.push_back(ctx); + if (!prepare_status.ok()) { + for (ExprContext* ctx: expr_ctxs) ctx->Close(&state); + (env)->ThrowNew(JniUtil::internal_exc_class(), prepare_status.GetDetail().c_str()); + return result_bytes; + } } // UDFs which cannot be interpreted need to be handled by codegen. @@ -116,33 +133,37 @@ Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs( codegen->FinalizeModule(); } + // Open() and evaluate the exprs. Always Close() the exprs even in case of errors. vector<TColumnValue> results; - // Open and evaluate the exprs. Also, always Close() the exprs even in case of errors. - for (int i = 0; i < expr_ctxs.size(); ++i) { - Status open_status = expr_ctxs[i]->Open(&state); - if (!open_status.ok()) { - for (int j = i; j < expr_ctxs.size(); ++j) { - expr_ctxs[j]->Close(&state); - } - (env)->ThrowNew(JniUtil::internal_exc_class(), open_status.GetDetail().c_str()); - return result_bytes; - } + Status status; + int i; + for (i = 0; i < expr_ctxs.size(); ++i) { + status = expr_ctxs[i]->Open(&state); + if (!status.ok()) break; + TColumnValue val; - expr_ctxs[i]->GetValue(NULL, false, &val); - // We check here if an error was set in the expression evaluated through GetValue() - // and throw an exception accordingly - Status getvalue_status = expr_ctxs[i]->root()->GetFnContextError(expr_ctxs[i]); - if (!getvalue_status.ok()) { - for (int j = i; j < expr_ctxs.size(); ++j) { - expr_ctxs[j]->Close(&state); - } - (env)->ThrowNew(JniUtil::internal_exc_class(), getvalue_status.GetDetail().c_str()); - return result_bytes; + expr_ctxs[i]->GetConstantValue(&val); + status = expr_ctxs[i]->root()->GetFnContextError(expr_ctxs[i]); + if (!status.ok()) break; + + // Check for mem limit exceeded. + status = state.CheckQueryState(); + if (!status.ok()) break; + // Check for warnings registered in the runtime state. + if (state.HasErrors()) { + status = Status(state.ErrorLog()); + break; } expr_ctxs[i]->Close(&state); results.push_back(val); } + // Convert status to exception. Close all remaining expr contexts. + if (!status.ok()) { + for (int j = i; j < expr_ctxs.size(); ++j) expr_ctxs[j]->Close(&state); + (env)->ThrowNew(JniUtil::internal_exc_class(), status.GetDetail().c_str()); + return result_bytes; + } expr_results.__set_colVals(results); THROW_IF_ERROR_RET(SerializeThriftMsg(env, &expr_results, &result_bytes), env, http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/common/thrift/Exprs.thrift ---------------------------------------------------------------------- diff --git a/common/thrift/Exprs.thrift b/common/thrift/Exprs.thrift index 3f91ac3..205f7bb 100644 --- a/common/thrift/Exprs.thrift +++ b/common/thrift/Exprs.thrift @@ -21,22 +21,22 @@ namespace java org.apache.impala.thrift include "Types.thrift" enum TExprNodeType { + NULL_LITERAL, BOOL_LITERAL, + INT_LITERAL, + FLOAT_LITERAL, + STRING_LITERAL, + DECIMAL_LITERAL, + TIMESTAMP_LITERAL, CASE_EXPR, COMPOUND_PRED, - FLOAT_LITERAL, - INT_LITERAL, IN_PRED, IS_NULL_PRED, LIKE_PRED, - LITERAL_PRED, - NULL_LITERAL, SLOT_REF, - STRING_LITERAL, TUPLE_IS_NULL_PRED, FUNCTION_CALL, AGGREGATE_EXPR, - DECIMAL_LITERAL, IS_NOT_EMPTY_PRED } @@ -67,6 +67,11 @@ struct TIntLiteral { 1: required i64 value } +struct TTimestampLiteral { + // 16-byte raw representation of a TimestampValue + 1: required binary value +} + // The units which can be used when extracting a Timestamp. TExtractField is never used // in any messages. This enum is here to provide a single definition that can be shared // by the front and backend. @@ -137,6 +142,7 @@ struct TExprNode { 16: optional TTupleIsNullPredicate tuple_is_null_pred 17: optional TDecimalLiteral decimal_literal 18: optional TAggregateExpr agg_expr + 19: 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/bbf5255d/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java index 3323ed4..a7f259d 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java +++ b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java @@ -31,6 +31,7 @@ import org.apache.impala.authorization.PrivilegeRequest; import org.apache.impala.catalog.AuthorizationException; import org.apache.impala.catalog.Db; import org.apache.impala.catalog.ImpaladCatalog; +import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.common.InternalException; import org.apache.impala.common.Pair; @@ -38,6 +39,7 @@ import org.apache.impala.rewrite.BetweenToCompoundRule; import org.apache.impala.rewrite.ExprRewriteRule; import org.apache.impala.rewrite.ExprRewriter; import org.apache.impala.rewrite.ExtractCommonConjunctRule; +import org.apache.impala.rewrite.FoldConstantsRule; import org.apache.impala.thrift.TAccessEvent; import org.apache.impala.thrift.TLineageGraph; import org.apache.impala.thrift.TQueryCtx; @@ -71,6 +73,7 @@ public class AnalysisContext { // BetweenPredicates should be rewritten first to help trigger other rules. List<ExprRewriteRule> rules = Lists.newArrayList(BetweenToCompoundRule.INSTANCE); if (queryCtx.getRequest().getQuery_options().enable_expr_rewrites) { + rules.add(FoldConstantsRule.INSTANCE); rules.add(ExtractCommonConjunctRule.INSTANCE); } rewriter_ = new ExprRewriter(rules); @@ -390,9 +393,24 @@ public class AnalysisContext { reAnalyze = true; } if (reAnalyze) { + // The rewrites should have no user-visible effect. Remember the original result + // types and column labels to restore them after the rewritten stmt has been + // reset() and re-analyzed. + List<Type> origResultTypes = Lists.newArrayList(); + for (Expr e: analysisResult_.stmt_.getResultExprs()) { + origResultTypes.add(e.getType()); + } + List<String> origColLabels = + Lists.newArrayList(analysisResult_.stmt_.getColLabels()); + + // Re-analyze the stmt with a new analyzer. analysisResult_.analyzer_ = new Analyzer(catalog_, queryCtx_, authzConfig_); analysisResult_.stmt_.reset(); analysisResult_.stmt_.analyze(analysisResult_.analyzer_); + + // Restore the original result types and column labels. + analysisResult_.stmt_.castResultExprs(origResultTypes); + analysisResult_.stmt_.setColLabels(origColLabels); LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql()); if (isExplain) analysisResult_.stmt_.setIsExplain(); Preconditions.checkState(!analysisResult_.requiresSubqueryRewrite()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 0638bed..fa03434 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -60,6 +60,7 @@ import org.apache.impala.common.PrintUtils; import org.apache.impala.planner.PlanNode; import org.apache.impala.rewrite.BetweenToCompoundRule; import org.apache.impala.rewrite.ExprRewriter; +import org.apache.impala.rewrite.FoldConstantsRule; import org.apache.impala.service.FeSupport; import org.apache.impala.thrift.TAccessEvent; import org.apache.impala.thrift.TCatalogObjectType; @@ -298,6 +299,10 @@ public class Analyzer { // TODO: Investigate what to do with other catalog objects. private final HashMap<TableName, Table> referencedTables_ = Maps.newHashMap(); + // Expr rewriter for foldinc constants. + private final ExprRewriter constantFolder_ = + new ExprRewriter(FoldConstantsRule.INSTANCE); + // Timeline of important events in the planning process, used for debugging / // profiling private final EventSequence timeline = new EventSequence("Planner Timeline"); @@ -655,6 +660,7 @@ public class Analyzer { } public TableRef getTableRef(TupleId tid) { return tableRefMap_.get(tid); } + public ExprRewriter getConstantFolder() { return globalState_.constantFolder_; } /** * Given a "table alias"."column alias", return the SlotDescriptor http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 78fff56..89e3cb9 100644 --- a/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java +++ b/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java @@ -335,12 +335,11 @@ public class BinaryPredicate extends Predicate { } /** - * Swaps the first with the second child in-place. Only valid to call for - * equivalence and not equal predicates. + * Swaps the first and second child in-place and sets the operation to its converse. */ public void reverse() { - Preconditions.checkState(op_.isEquivalence() || op_ == Operator.NE); Collections.swap(children_, 0, 1); + op_ = op_.converse(); } @Override http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java b/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java index 9d7b2fd..0198704 100644 --- a/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java +++ b/fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java @@ -22,6 +22,7 @@ import org.apache.impala.common.AnalysisException; import org.apache.impala.thrift.TBoolLiteral; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; + import com.google.common.base.Objects; public class BoolLiteral extends LiteralExpr { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java b/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java index 5dca6b5..cd7b1c8 100644 --- a/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java @@ -18,8 +18,8 @@ package org.apache.impala.analysis; import java.util.Collections; -import java.util.List; import java.util.EnumSet; +import java.util.List; import org.apache.impala.authorization.Privilege; import org.apache.impala.catalog.Db; @@ -27,6 +27,7 @@ import org.apache.impala.catalog.HdfsTable; import org.apache.impala.catalog.KuduTable; import org.apache.impala.catalog.MetaStoreClientPool.MetaStoreClient; import org.apache.impala.catalog.Table; +import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.rewrite.ExprRewriter; import org.apache.impala.service.CatalogOpExecutor; @@ -212,6 +213,18 @@ public class CreateTableAsSelectStmt extends StatementBase { } @Override + public List<Expr> getResultExprs() { return insertStmt_.getResultExprs(); } + + @Override + public void castResultExprs(List<Type> types) throws AnalysisException { + super.castResultExprs(types); + // Set types of column definitions. + List<ColumnDef> colDefs = createStmt_.getColumnDefs(); + Preconditions.checkState(colDefs.size() == types.size()); + for (int i = 0; i < types.size(); ++i) colDefs.get(i).setType(types.get(i)); + } + + @Override public void rewriteExprs(ExprRewriter rewriter) throws AnalysisException { Preconditions.checkState(isAnalyzed()); insertStmt_.rewriteExprs(rewriter); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 fd7f86f..779e252 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Expr.java +++ b/fe/src/main/java/org/apache/impala/analysis/Expr.java @@ -227,6 +227,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl children_ = Expr.cloneList(other.children_); } + public boolean isAnalyzed() { return isAnalyzed_; } public ExprId getId() { return id_; } protected void setId(ExprId id) { id_ = id; } public Type getType() { return type_; } @@ -1199,36 +1200,6 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl } /** - * For children of 'this' that are constant expressions and the type of which has a - * LiteralExpr subclass, evaluate them in the BE and substitute the child with the - * resulting LiteralExpr. Modifies 'this' in place. If any children are folded, this - * Expr is reset and re-analyzed. - * - * Throws an AnalysisException if the evaluation fails in the BE. - * - * TODO: Convert to a generic constant expr folding function to be used during analysis. - */ - public void foldConstantChildren(Analyzer analyzer) throws AnalysisException { - Preconditions.checkState(isAnalyzed_); - Preconditions.checkNotNull(analyzer); - - int numFolded = 0; - for (int i = 0; i < children_.size(); ++i) { - Expr child = getChild(i); - if (child.isLiteral() || !child.isConstant()) continue; - LiteralExpr literalExpr = LiteralExpr.create(child, analyzer.getQueryCtx()); - if (literalExpr == null) continue; - setChild(i, literalExpr); - ++numFolded; - } - - if (numFolded > 0) { - reset(); - analyze(analyzer); - } - } - - /** * Returns true iff all of this Expr's children have their costs set. */ protected boolean hasChildCosts() { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 2653684..19f0e49 100644 --- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java +++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java @@ -33,6 +33,7 @@ import org.apache.impala.thrift.TAggregateExpr; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; import org.apache.impala.thrift.TFunctionBinaryType; + import com.google.common.base.Joiner; import com.google.common.base.Objects; import com.google.common.base.Preconditions; @@ -231,12 +232,22 @@ public class FunctionCallExpr extends Expr { } } - /** - * Aggregate functions are never constant. - */ @Override public boolean isConstant() { - if (fn_ != null && fn_ instanceof AggregateFunction) return false; + // 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. + List<String> path = fnName_.getFnNamePath(); + fnName = path.get(path.size() - 1); + } + // Non-deterministic functions are never constant. + if (fnName.equalsIgnoreCase("rand") || fnName.equalsIgnoreCase("random")) { + return false; + } + // Sleep is a special function for testing. + if (fnName.equalsIgnoreCase("sleep")) return false; return super.isConstant(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 4598638..8ce4497 100644 --- a/fe/src/main/java/org/apache/impala/analysis/InPredicate.java +++ b/fe/src/main/java/org/apache/impala/analysis/InPredicate.java @@ -17,7 +17,6 @@ package org.apache.impala.analysis; -import java.util.ArrayList; import java.util.List; import org.apache.impala.catalog.Db; @@ -29,6 +28,7 @@ import org.apache.impala.common.AnalysisException; import org.apache.impala.common.Reference; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; + import com.google.common.base.Preconditions; import com.google.common.collect.Lists; @@ -125,7 +125,7 @@ public class InPredicate extends Predicate { // the subquery are type compatible. No need to perform any // casting at this point. Any casting needed will be performed when the // subquery is unnested. - ArrayList<Expr> subqueryExprs = subquery.getStatement().getResultExprs(); + List<Expr> subqueryExprs = subquery.getStatement().getResultExprs(); Expr compareExpr = children_.get(0); Expr subqueryExpr = subqueryExprs.get(0); analyzer.getCompatibleType(compareExpr.getType(), compareExpr, subqueryExpr); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java b/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java index 5528da9..4ae253e 100644 --- a/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java @@ -237,8 +237,6 @@ public class InsertStmt extends StatementBase { // views and to ignore irrelevant ORDER BYs. Analyzer queryStmtAnalyzer = new Analyzer(analyzer); queryStmt_.analyze(queryStmtAnalyzer); - // Subqueries need to be rewritten by the StmtRewriter first. - if (analyzer.containsSubquery()) return; // Use getResultExprs() and not getBaseTblResultExprs() here because the final // substitution with TupleIsNullPredicate() wrapping happens in planning. selectListExprs = Expr.cloneList(queryStmt_.getResultExprs()); @@ -645,7 +643,7 @@ public class InsertStmt extends StatementBase { // tableColumns is guaranteed to exist after the earlier analysis checks Column tableColumn = table_.getColumn(pkv.getColName()); Expr compatibleExpr = checkTypeCompatibility( - targetTableName_.toString(), tableColumn, pkv.getValue()); + targetTableName_.toString(), tableColumn, pkv.getLiteralValue()); tmpPartitionKeyExprs.add(compatibleExpr); tmpPartitionKeyNames.add(pkv.getColName()); } @@ -764,6 +762,9 @@ public class InsertStmt extends StatementBase { } @Override + public ArrayList<Expr> getResultExprs() { return resultExprs_; } + + @Override public void rewriteExprs(ExprRewriter rewriter) throws AnalysisException { Preconditions.checkState(isAnalyzed()); queryStmt_.rewriteExprs(rewriter); @@ -786,7 +787,6 @@ public class InsertStmt extends StatementBase { public boolean hasShuffleHint() { return hasShuffleHint_; } public boolean hasNoShuffleHint() { return hasNoShuffleHint_; } public boolean hasClusteredHint() { return hasClusteredHint_; } - public ArrayList<Expr> getResultExprs() { return resultExprs_; } public ArrayList<Expr> getPrimaryKeyExprs() { return primaryKeyExprs_; } public DataSink createDataSink() { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 4ace2dc..c1d41cb 100644 --- a/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java +++ b/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java @@ -17,6 +17,7 @@ package org.apache.impala.analysis; +import java.io.UnsupportedEncodingException; import java.math.BigDecimal; import java.math.BigInteger; @@ -29,6 +30,9 @@ import org.apache.impala.service.FeSupport; import org.apache.impala.thrift.TColumnValue; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TQueryCtx; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Preconditions; /** @@ -36,6 +40,7 @@ import com.google.common.base.Preconditions; * ordering of HdfsPartitions whose partition-key values are represented as literals. */ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr> { + private final static Logger LOG = LoggerFactory.getLogger(LiteralExpr.class); public LiteralExpr() { numDistinctValues_ = 1; @@ -153,7 +158,8 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr * Assumes expr has been analyzed. Returns constExpr if is it already a LiteralExpr. * Returns null for types that do not have a LiteralExpr subclass, e.g. TIMESTAMP, or * in cases where the corresponding LiteralExpr is not able to represent the evaluation - * result, e.g., NaN or infinity. + * result, e.g., NaN or infinity. Returns null if the expr evaluation encountered errors + * or warnings in the BE. * TODO: Support non-scalar types. */ public static LiteralExpr create(Expr constExpr, TQueryCtx queryCtx) @@ -166,8 +172,9 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr try { val = FeSupport.EvalConstExpr(constExpr, queryCtx); } catch (InternalException e) { - throw new AnalysisException(String.format("Failed to evaluate expr '%s'", - constExpr.toSql()), e); + LOG.debug(String.format("Failed to evaluate expr '%s'", + constExpr.toSql(), e.getMessage())); + return null; } LiteralExpr result = null; @@ -215,11 +222,32 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr case STRING: case VARCHAR: case CHAR: - if (val.isSetString_val()) result = new StringLiteral(val.string_val); + if (val.isSetBinary_val()) { + byte[] bytes = new byte[val.binary_val.remaining()]; + val.binary_val.get(bytes); + // Converting strings between the BE/FE does not work properly for the + // extended ASCII characters above 127. Bail in such cases to avoid + // producing incorrect results. + for (byte b: bytes) if (b < 0) return null; + try { + // US-ASCII is 7-bit ASCII. + String strVal = new String(bytes, "US-ASCII"); + // The evaluation result is a raw string that must not be unescaped. + result = new StringLiteral(strVal, constExpr.getType(), false); + } catch (UnsupportedEncodingException e) { + return null; + } + } + break; + case TIMESTAMP: + // Expects both the binary and string fields to be set, so we get the raw + // representation as well as the string representation. + if (val.isSetBinary_val() && val.isSetString_val()) { + result = new TimestampLiteral(val.getBinary_val(), val.getString_val()); + } break; case DATE: case DATETIME: - case TIMESTAMP: return null; default: Preconditions.checkState(false, @@ -229,7 +257,7 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr // None of the fields in the thrift struct were set indicating a NULL. if (result == null) result = new NullLiteral(); - result.analyze(null); + result.analyzeNoThrow(null); return (LiteralExpr)result; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java b/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java index 0107079..0e94b29 100644 --- a/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java @@ -29,6 +29,7 @@ import org.apache.impala.authorization.PrivilegeRequestBuilder; import org.apache.impala.catalog.Column; import org.apache.impala.catalog.KuduTable; import org.apache.impala.catalog.Table; +import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.common.Pair; import org.apache.impala.planner.DataSink; @@ -284,6 +285,14 @@ public abstract class ModifyStmt extends StatementBase { } @Override + public List<Expr> getResultExprs() { return sourceStmt_.getResultExprs(); } + + @Override + public void castResultExprs(List<Type> types) throws AnalysisException { + sourceStmt_.castResultExprs(types); + } + + @Override public void rewriteExprs(ExprRewriter rewriter) throws AnalysisException { Preconditions.checkState(isAnalyzed()); for (Pair<SlotRef, Expr> assignment: assignments_) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java b/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java index ef88154..0dcb264 100644 --- a/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java +++ b/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java @@ -20,6 +20,7 @@ package org.apache.impala.analysis; import org.apache.impala.catalog.Type; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; + import com.google.common.base.Objects; import com.google.common.base.Preconditions; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java b/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java index b41f2f9..6de2d93 100644 --- a/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java +++ b/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java @@ -29,6 +29,7 @@ import org.apache.impala.common.InternalException; import org.apache.impala.common.Reference; import org.apache.impala.planner.HdfsPartitionPruner; import org.apache.impala.thrift.TPartitionKeyValue; + import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -66,7 +67,7 @@ public class PartitionSet extends PartitionSpecBase { TupleDescriptor desc = analyzer.getDescriptor(tableName_.toString()); List<SlotId> partitionSlots = desc.getPartitionSlots(); for (Expr e: conjuncts) { - e.foldConstantChildren(analyzer); + analyzer.getConstantFolder().rewrite(e, analyzer); // Make sure there are no constant predicates in the partition exprs. if (e.isConstant()) { throw new AnalysisException(String.format("Invalid partition expr %s. A " + http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java b/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java index 464b5a5..135f2e4 100644 --- a/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java @@ -24,9 +24,9 @@ import java.util.Set; import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; -import org.apache.impala.common.TreeNode; import org.apache.impala.planner.DataSink; import org.apache.impala.planner.PlanRootSink; + import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.collect.Lists; @@ -327,11 +327,6 @@ public abstract class QueryStmt extends StatementBase { } /** - * UnionStmt and SelectStmt have different implementations. - */ - public abstract ArrayList<String> getColLabels(); - - /** * Returns the materialized tuple ids of the output of this stmt. * Used in case this stmt is part of an @InlineViewRef, * since we need to know the materialized tupls ids of a TableRef. @@ -347,6 +342,9 @@ public abstract class QueryStmt extends StatementBase { */ public abstract void collectTableRefs(List<TableRef> tblRefs); + @Override + public List<Expr> getResultExprs() { return resultExprs_; } + public void setWithClause(WithClause withClause) { this.withClause_ = withClause; } public boolean hasWithClause() { return withClause_ != null; } public WithClause getWithClause() { return withClause_; } @@ -357,7 +355,6 @@ public abstract class QueryStmt extends StatementBase { public long getOffset() { return limitElement_.getOffset(); } public SortInfo getSortInfo() { return sortInfo_; } public boolean evaluateOrderBy() { return evaluateOrderBy_; } - public ArrayList<Expr> getResultExprs() { return resultExprs_; } public ArrayList<Expr> getBaseTblResultExprs() { return baseTblResultExprs_; } public void setLimit(long limit) throws AnalysisException { Preconditions.checkState(limit >= 0); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java index b112083..d3dc8d2 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java @@ -110,6 +110,9 @@ public class SelectStmt extends QueryStmt { */ public Expr getHavingPred() { return havingPred_; } + @Override + public List<String> getColLabels() { return colLabels_; } + public List<TableRef> getTableRefs() { return fromClause_.getTableRefs(); } public boolean hasWhereClause() { return whereClause_ != null; } public boolean hasGroupByClause() { return groupingExprs_ != null; } @@ -120,8 +123,6 @@ public class SelectStmt extends QueryStmt { public AnalyticInfo getAnalyticInfo() { return analyticInfo_; } public boolean hasAnalyticInfo() { return analyticInfo_ != null; } public boolean hasHavingClause() { return havingClause_ != null; } - @Override - public ArrayList<String> getColLabels() { return colLabels_; } public ExprSubstitutionMap getBaseTblSmap() { return baseTblSmap_; } // Column alias generator used during query rewriting. @@ -879,7 +880,7 @@ public class SelectStmt extends QueryStmt { if (havingClause_ != null) { havingClause_ = rewriter.rewrite(havingClause_, analyzer_); } - if (groupingExprs_ != null) rewriter.applyList(groupingExprs_, analyzer_); + if (groupingExprs_ != null) rewriter.rewriteList(groupingExprs_, analyzer_); if (orderByElements_ != null) { for (OrderByElement orderByElem: orderByElements_) { orderByElem.setExpr(rewriter.rewrite(orderByElem.getExpr(), analyzer_)); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 36771f3..1d1e319 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SlotRef.java +++ b/fe/src/main/java/org/apache/impala/analysis/SlotRef.java @@ -21,9 +21,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Set; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import org.apache.impala.analysis.Path.PathType; import org.apache.impala.catalog.Table; import org.apache.impala.catalog.TableLoadingException; @@ -32,6 +29,9 @@ import org.apache.impala.common.AnalysisException; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; import org.apache.impala.thrift.TSlotRef; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Joiner; import com.google.common.base.Objects; import com.google.common.base.Preconditions; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/StatementBase.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java index b4e3155..bd8e8e2 100644 --- a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java +++ b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java @@ -17,12 +17,17 @@ package org.apache.impala.analysis; +import java.util.Collections; +import java.util.List; + import org.apache.commons.lang.NotImplementedException; import org.apache.impala.catalog.Column; import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.rewrite.ExprRewriter; +import com.google.common.base.Preconditions; + /** * Base class for all Impala SQL statements. */ @@ -64,6 +69,47 @@ abstract class StatementBase implements ParseNode { } /** + * Returns the output column labels of this statement, if applicable, or an empty list + * if not applicable (not all statements produce an output result set). + * Subclasses must override this as necessary. + */ + public List<String> getColLabels() { return Collections.<String>emptyList(); } + + /** + * Sets the column labels of this statement, if applicable. No-op of the statement does + * not produce an output result set. + */ + public void setColLabels(List<String> colLabels) { + List<String> oldLabels = getColLabels(); + if (oldLabels == colLabels) return; + oldLabels.clear(); + oldLabels.addAll(colLabels); + } + + /** + * Returns the unresolved result expressions of this statement, if applicable, or an + * empty list if not applicable (not all statements produce an output result set). + * Subclasses must override this as necessary. + */ + public List<Expr> getResultExprs() { return Collections.<Expr>emptyList(); } + + /** + * Casts the result expressions and derived members (e.g., destination column types for + * CTAS) to the given types. No-op if this statement does not have result expressions. + * Throws when casting fails. Subclasses may override this as necessary. + */ + public void castResultExprs(List<Type> types) throws AnalysisException { + List<Expr> resultExprs = getResultExprs(); + Preconditions.checkNotNull(resultExprs); + Preconditions.checkState(resultExprs.size() == types.size()); + for (int i = 0; i < types.size(); ++i) { + if (!resultExprs.get(i).getType().equals(types.get(i))) { + resultExprs.set(i, resultExprs.get(i).castTo(types.get(i))); + } + } + } + + /** * Uses the given 'rewriter' to transform all Exprs in this statement according * to the rules specified in the 'rewriter'. Replaces the original Exprs with the * transformed ones in-place. Subclasses that have Exprs to be rewritten must http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java b/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java index 4cca49e..4f838ff 100644 --- a/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java +++ b/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java @@ -21,32 +21,34 @@ import java.io.IOException; import java.io.StringReader; import java.math.BigDecimal; -import java_cup.runtime.Symbol; - import org.apache.hadoop.hive.ql.parse.BaseSemanticAnalyzer; - import org.apache.impala.catalog.ScalarType; import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; import org.apache.impala.thrift.TStringLiteral; + import com.google.common.base.Objects; import com.google.common.base.Preconditions; +import java_cup.runtime.Symbol; + public class StringLiteral extends LiteralExpr { private final String value_; + // Indicates whether this value needs to be unescaped in toThrift(). + private final boolean needsUnescaping_; + public StringLiteral(String value) { - this.value_ = value; - type_ = ScalarType.STRING; - evalCost_ = LITERAL_COST; + this(value, ScalarType.STRING, true); } - public StringLiteral(String value, Type type) { - this.value_ = value; + public StringLiteral(String value, Type type, boolean needsUnescaping) { + value_ = value; type_ = type; evalCost_ = LITERAL_COST; + needsUnescaping_ = needsUnescaping; } /** @@ -55,26 +57,27 @@ public class StringLiteral extends LiteralExpr { protected StringLiteral(StringLiteral other) { super(other); value_ = other.value_; + needsUnescaping_ = other.needsUnescaping_; } @Override public boolean equals(Object obj) { if (!super.equals(obj)) return false; - return ((StringLiteral) obj).value_.equals(value_); + StringLiteral other = (StringLiteral) obj; + return needsUnescaping_ == other.needsUnescaping_ && value_.equals(other.value_); } @Override public int hashCode() { return value_.hashCode(); } @Override - public String toSqlImpl() { - return "'" + value_ + "'"; - } + public String toSqlImpl() { return "'" + value_ + "'"; } @Override protected void toThrift(TExprNode msg) { msg.node_type = TExprNodeType.STRING_LITERAL; - msg.string_literal = new TStringLiteral(getUnescapedValue()); + String val = (needsUnescaping_) ? getUnescapedValue() : value_; + msg.string_literal = new TStringLiteral(val); } public String getValue() { return value_; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/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 414b98d..afb839e 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Subquery.java +++ b/fe/src/main/java/org/apache/impala/analysis/Subquery.java @@ -18,6 +18,7 @@ package org.apache.impala.analysis; import java.util.ArrayList; +import java.util.List; import org.apache.hadoop.hive.metastore.MetaStoreUtils; import org.apache.impala.catalog.ArrayType; @@ -87,7 +88,7 @@ public class Subquery extends Expr { // Set the subquery type based on the types of the exprs in the // result list of the associated SelectStmt. - ArrayList<Expr> stmtResultExprs = stmt_.getResultExprs(); + List<Expr> stmtResultExprs = stmt_.getResultExprs(); if (stmtResultExprs.size() == 1) { type_ = stmtResultExprs.get(0).getType(); Preconditions.checkState(!type_.isComplexType()); @@ -109,7 +110,7 @@ public class Subquery extends Expr { * Check if the subquery's SelectStmt returns a single column of scalar type. */ public boolean returnsScalarColumn() { - ArrayList<Expr> stmtResultExprs = stmt_.getResultExprs(); + List<Expr> stmtResultExprs = stmt_.getResultExprs(); if (stmtResultExprs.size() == 1 && stmtResultExprs.get(0).getType().isScalarType()) { return true; } @@ -120,10 +121,10 @@ public class Subquery extends Expr { * Create a StrucType from the result expr list of a subquery's SelectStmt. */ private StructType createStructTypeFromExprList() { - ArrayList<Expr> stmtResultExprs = stmt_.getResultExprs(); + List<Expr> stmtResultExprs = stmt_.getResultExprs(); ArrayList<StructField> structFields = Lists.newArrayList(); // Check if we have unique labels - ArrayList<String> labels = stmt_.getColLabels(); + List<String> labels = stmt_.getColLabels(); boolean hasUniqueLabels = true; if (Sets.newHashSet(labels).size() != labels.size()) hasUniqueLabels = false; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java b/fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java new file mode 100644 index 0000000..8f07082 --- /dev/null +++ b/fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.analysis; + +import java.util.Arrays; + +import org.apache.impala.catalog.Type; +import org.apache.impala.common.AnalysisException; +import org.apache.impala.thrift.TExprNode; +import org.apache.impala.thrift.TExprNodeType; +import org.apache.impala.thrift.TTimestampLiteral; +import org.apache.kudu.client.shaded.com.google.common.base.Preconditions; + +/** + * Represents a literal timestamp. Its value is a 16-byte array that corresponds to a + * raw BE TimestampValue, e.g., in a slot. In addition, it stores the string + * representation of the timestamp value to avoid converting the raw bytes on the Java + * side. Such a conversion could potentially be inconsistent with what the BE would + * produce, so it's better to defer to a single source of truth (the BE implementation). + * + * Literal timestamps can currently only be created via constant folding. There is no + * way to directly specify a literal timestamp from SQL. + */ +public class TimestampLiteral extends LiteralExpr { + private final byte[] value_; + private final String strValue_; + + public TimestampLiteral(byte[] value, String strValue) { + Preconditions.checkState(value.length == Type.TIMESTAMP.getSlotSize()); + value_ = value; + strValue_ = strValue; + type_ = Type.TIMESTAMP; + } + + /** + * Copy c'tor used in clone. + */ + protected TimestampLiteral(TimestampLiteral other) { + super(other); + value_ = Arrays.copyOf(other.value_, other.value_.length); + strValue_ = other.strValue_; + } + + @Override + public boolean equals(Object obj) { + if (!super.equals(obj)) return false; + return Arrays.equals(value_, ((TimestampLiteral) obj).value_); + } + + @Override + public int hashCode() { return Arrays.hashCode(value_); } + + @Override + public String toSqlImpl() { + // ANSI Timestamp Literal format. + return "TIMESTAMP '" + getStringValue() + "'"; + } + + @Override + public String getStringValue() { return strValue_; } + + public byte[] getValue() { return value_; } + + @Override + protected void toThrift(TExprNode msg) { + msg.node_type = TExprNodeType.TIMESTAMP_LITERAL; + msg.timestamp_literal = new TTimestampLiteral(); + msg.timestamp_literal.setValue(value_); + } + + @Override + protected Expr uncheckedCastTo(Type targetType) throws AnalysisException { + if (targetType.equals(type_)) { + return this; + } else { + return new CastExpr(targetType, this); + } + } + + @Override + public int compareTo(LiteralExpr o) { + int ret = super.compareTo(o); + if (ret != 0) return ret; + TimestampLiteral other = (TimestampLiteral) o; + return strValue_.compareTo(other.strValue_); + } + + @Override + public Expr clone() { return new TimestampLiteral(this); } +} http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java b/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java index 674e399..ad9150f 100644 --- a/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java @@ -598,7 +598,7 @@ public class UnionStmt extends QueryStmt { } @Override - public ArrayList<String> getColLabels() { + public List<String> getColLabels() { Preconditions.checkState(operands_.size() > 0); return operands_.get(0).getQueryStmt().getColLabels(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java b/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java index 89296f1..e8b26bc 100644 --- a/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java @@ -28,15 +28,12 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.HRegionLocation; -import org.apache.hadoop.hbase.client.HTable; import org.apache.hadoop.hbase.filter.CompareFilter; import org.apache.hadoop.hbase.util.Bytes; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import org.apache.impala.analysis.Analyzer; import org.apache.impala.analysis.BinaryPredicate; import org.apache.impala.analysis.Expr; +import org.apache.impala.analysis.LiteralExpr; import org.apache.impala.analysis.SlotDescriptor; import org.apache.impala.analysis.StringLiteral; import org.apache.impala.analysis.TupleDescriptor; @@ -45,10 +42,7 @@ import org.apache.impala.catalog.HBaseTable; import org.apache.impala.catalog.PrimitiveType; import org.apache.impala.catalog.Type; import org.apache.impala.common.ImpalaException; -import org.apache.impala.common.InternalException; import org.apache.impala.common.Pair; -import org.apache.impala.service.FeSupport; -import org.apache.impala.thrift.TColumnValue; import org.apache.impala.thrift.TExplainLevel; import org.apache.impala.thrift.THBaseFilter; import org.apache.impala.thrift.THBaseKeyRange; @@ -60,6 +54,9 @@ import org.apache.impala.thrift.TQueryOptions; import org.apache.impala.thrift.TScanRange; import org.apache.impala.thrift.TScanRangeLocation; import org.apache.impala.thrift.TScanRangeLocationList; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; @@ -144,7 +141,7 @@ public class HBaseScanNode extends ScanNode { * are always encded as ascii. * ValueRange is null if there is no predicate on the row-key. */ - private void setStartStopKey(Analyzer analyzer) throws InternalException { + private void setStartStopKey(Analyzer analyzer) throws ImpalaException { Preconditions.checkNotNull(keyRanges_); Preconditions.checkState(keyRanges_.size() == 1); @@ -154,30 +151,32 @@ public class HBaseScanNode extends ScanNode { Preconditions.checkState(rowRange.getLowerBound().isConstant()); Preconditions.checkState( rowRange.getLowerBound().getType().equals(Type.STRING)); - TColumnValue val = FeSupport.EvalConstExpr(rowRange.getLowerBound(), + LiteralExpr val = LiteralExpr.create(rowRange.getLowerBound(), analyzer.getQueryCtx()); - if (!val.isSetString_val()) { + if (val instanceof StringLiteral) { + StringLiteral litVal = (StringLiteral) val; + startKey_ = convertToBytes(litVal.getStringValue(), + !rowRange.getLowerBoundInclusive()); + } else { // lower bound is null. isEmpty_ = true; return; - } else { - startKey_ = convertToBytes(val.getString_val(), - !rowRange.getLowerBoundInclusive()); } } if (rowRange.getUpperBound() != null) { Preconditions.checkState(rowRange.getUpperBound().isConstant()); Preconditions.checkState( rowRange.getUpperBound().getType().equals(Type.STRING)); - TColumnValue val = FeSupport.EvalConstExpr(rowRange.getUpperBound(), + LiteralExpr val = LiteralExpr.create(rowRange.getUpperBound(), analyzer.getQueryCtx()); - if (!val.isSetString_val()) { - // upper bound is null. + if (val instanceof StringLiteral) { + StringLiteral litVal = (StringLiteral) val; + stopKey_ = convertToBytes(litVal.getStringValue(), + rowRange.getUpperBoundInclusive()); + } else { + // lower bound is null. isEmpty_ = true; return; - } else { - stopKey_ = convertToBytes(val.getString_val(), - rowRange.getUpperBoundInclusive()); } } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java b/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java index f719d0c..b8eb7ed 100644 --- a/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java +++ b/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java @@ -36,7 +36,6 @@ import org.apache.impala.analysis.InPredicate; import org.apache.impala.analysis.IsNullPredicate; import org.apache.impala.analysis.LiteralExpr; import org.apache.impala.analysis.NullLiteral; -import org.apache.impala.analysis.SlotDescriptor; import org.apache.impala.analysis.SlotId; import org.apache.impala.analysis.SlotRef; import org.apache.impala.analysis.TupleDescriptor; @@ -174,7 +173,7 @@ public class HdfsPartitionPruner { if (expr instanceof BinaryPredicate) { // Evaluate any constant expression in the BE try { - expr.foldConstantChildren(analyzer); + analyzer.getConstantFolder().rewrite(expr, analyzer); } catch (AnalysisException e) { LOG.error("Error evaluating constant expressions in the BE: " + e.getMessage()); return false; @@ -198,7 +197,7 @@ public class HdfsPartitionPruner { } else if (expr instanceof InPredicate) { // Evaluate any constant expressions in the BE try { - expr.foldConstantChildren(analyzer); + analyzer.getConstantFolder().rewrite(expr, analyzer); } catch (AnalysisException e) { LOG.error("Error evaluating constant expressions in the BE: " + e.getMessage()); return false; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java b/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java index 6f20abd..61f6b28 100644 --- a/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java @@ -34,7 +34,6 @@ import org.apache.impala.analysis.SlotRef; import org.apache.impala.analysis.StringLiteral; import org.apache.impala.analysis.TupleDescriptor; import org.apache.impala.catalog.KuduTable; -import org.apache.impala.common.AnalysisException; import org.apache.impala.common.ImpalaRuntimeException; import org.apache.impala.thrift.TExplainLevel; import org.apache.impala.thrift.TKuduScanNode; @@ -363,19 +362,11 @@ public class KuduScanNode extends ScanNode { /** * Normalizes and returns a copy of 'predicate' consisting of an uncast SlotRef and a - * constant Expr into the following form: <SlotRef> <Op> <LiteralExpr> - * If 'predicate' cannot be expressed in this way, null is returned. + * constant Expr into the following form: <SlotRef> <Op> <LiteralExpr>. + * Assumes that constant expressions have already been folded. */ private static BinaryPredicate normalizeSlotRefComparison(BinaryPredicate predicate, Analyzer analyzer) { - try { - predicate = (BinaryPredicate) predicate.clone(); - predicate.foldConstantChildren(analyzer); - } catch (AnalysisException ex) { - // Throws if the expression cannot be evaluated by the BE. - return null; - } - SlotRef ref = null; if (predicate.getChild(0) instanceof SlotRef) { ref = (SlotRef) predicate.getChild(0); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/planner/Planner.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/Planner.java b/fe/src/main/java/org/apache/impala/planner/Planner.java index acfafb3..a1566b0 100644 --- a/fe/src/main/java/org/apache/impala/planner/Planner.java +++ b/fe/src/main/java/org/apache/impala/planner/Planner.java @@ -20,7 +20,6 @@ package org.apache.impala.planner; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Set; import org.apache.impala.analysis.AnalysisContext; import org.apache.impala.analysis.Analyzer; @@ -30,7 +29,6 @@ import org.apache.impala.analysis.ExprSubstitutionMap; import org.apache.impala.analysis.InsertStmt; import org.apache.impala.analysis.JoinOperator; import org.apache.impala.analysis.QueryStmt; -import org.apache.impala.analysis.SlotRef; import org.apache.impala.analysis.SortInfo; import org.apache.impala.catalog.HBaseTable; import org.apache.impala.catalog.KuduTable; @@ -38,7 +36,6 @@ import org.apache.impala.catalog.Table; import org.apache.impala.common.ImpalaException; import org.apache.impala.common.PrintUtils; import org.apache.impala.common.RuntimeEnv; -import org.apache.impala.common.TreeNode; import org.apache.impala.service.BackendConfig; import org.apache.impala.thrift.TExplainLevel; import org.apache.impala.thrift.TQueryCtx; @@ -51,9 +48,7 @@ import org.slf4j.LoggerFactory; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; -import com.google.common.base.Predicates; import com.google.common.collect.Lists; -import com.google.common.collect.Sets; /** * Creates an executable plan from an analyzed parse tree and query options. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java b/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java index 6219127..f62c236 100644 --- a/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java +++ b/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java @@ -898,7 +898,7 @@ public class SingleNodePlanner { private PlanNode createConstantSelectPlan(SelectStmt selectStmt, Analyzer analyzer) throws InternalException { Preconditions.checkState(selectStmt.getTableRefs().isEmpty()); - ArrayList<Expr> resultExprs = selectStmt.getResultExprs(); + List<Expr> resultExprs = selectStmt.getResultExprs(); // Create tuple descriptor for materialized tuple. TupleDescriptor tupleDesc = createResultTupleDescriptor(selectStmt, "union", analyzer); UnionNode unionNode = new UnionNode(ctx_.getNextNodeId(), tupleDesc.getId()); @@ -925,8 +925,8 @@ public class SingleNodePlanner { debugName); tupleDesc.setIsMaterialized(true); - ArrayList<Expr> resultExprs = selectStmt.getResultExprs(); - ArrayList<String> colLabels = selectStmt.getColLabels(); + List<Expr> resultExprs = selectStmt.getResultExprs(); + List<String> colLabels = selectStmt.getColLabels(); for (int i = 0; i < resultExprs.size(); ++i) { Expr resultExpr = resultExprs.get(i); String colLabel = colLabels.get(i); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java b/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java index bbd7c38..b352af7 100644 --- a/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java +++ b/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java @@ -27,6 +27,11 @@ import org.apache.impala.common.AnalysisException; * An ExprRewriteRule is intended to apply its transformation on a single Expr and not * recursively on all its children. The recursive and repeated application of * ExprRewriteRules is driven by an ExprRewriter. + * An ExprRewriteRule should preserve the type of the transformed expression to avoid + * having to re-analyze its parent (if any). This behavior guarantees that the Expr + * tree remains valid even if arbitrary subexpressions are transformed. It also means + * that the transformed expression may have a wider type than necessary. Callers are + * free to reset() and analyze() the result if they desire the minimal type. */ public interface ExprRewriteRule { /** http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java b/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java index 906fc0e..b9fbc71 100644 --- a/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java +++ b/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java @@ -87,7 +87,7 @@ public class ExprRewriter { return rewrittenExpr; } - public void applyList(List<Expr> exprs, Analyzer analyzer) throws AnalysisException { + public void rewriteList(List<Expr> exprs, Analyzer analyzer) throws AnalysisException { for (int i = 0; i < exprs.size(); ++i) exprs.set(i, rewrite(exprs.get(i), analyzer)); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java b/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java new file mode 100644 index 0000000..f3eb9a8 --- /dev/null +++ b/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java @@ -0,0 +1,63 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.rewrite; + +import org.apache.impala.analysis.Analyzer; +import org.apache.impala.analysis.Expr; +import org.apache.impala.analysis.LiteralExpr; +import org.apache.impala.common.AnalysisException; + +/** + * This rule replaces a constant Expr with its equivalent LiteralExpr by evaluating the + * Expr in the BE. Exprs that are already LiteralExprs are not changed. + * + * TODO: Expressions fed into this rule are currently not required to be analyzed + * in order to support constant folding in expressions that contain unresolved + * references to select-list aliases (such expressions cannot be analyzed). + * For sanity, we should restructure our analysis/rewriting to only allow analyzed exprs + * to be rewritten. + * + * Examples: + * 1 + 1 + 1 --> 3 + * toupper('abc') --> 'ABC' + * cast('2016-11-09' as timestamp) --> TIMESTAMP '2016-11-09 00:00:00' + */ +public class FoldConstantsRule implements ExprRewriteRule { + public static ExprRewriteRule INSTANCE = new FoldConstantsRule(); + + @Override + public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException { + // Avoid calling Expr.isConstant() because that would lead to repeated traversals + // of the Expr tree. Assumes the bottom-up application of this rule. Constant + // children should have been folded at this point. + for (Expr child: expr.getChildren()) if (!child.isLiteral()) return expr; + if (expr.isLiteral() || !expr.isConstant()) return expr; + // Analyze constant exprs, if necessary. Note that the 'expr' may become non-constant + // after analysis (e.g., aggregate functions). + if (!expr.isAnalyzed()) { + expr.analyze(analyzer); + if (!expr.isConstant()) return expr; + } + Expr result = LiteralExpr.create(expr, analyzer.getQueryCtx()); + // Preserve original type so parent Exprs do not need to be re-analyzed. + if (result != null) return result.castTo(expr.getType()); + return expr; + } + + private FoldConstantsRule() {} +} http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bbf5255d/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java index 2b77f39..6bc493b 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java @@ -1242,7 +1242,7 @@ public class AnalyzeExprsTest extends AnalyzerTest { SelectStmt select = (SelectStmt) AnalyzesOk(queryStr); Expr expr = null; if (arithmeticMode) { - ArrayList<Expr> selectListExprs = select.getResultExprs(); + List<Expr> selectListExprs = select.getResultExprs(); assertNotNull(selectListExprs); assertEquals(selectListExprs.size(), 1); // check the first expr in select list @@ -1270,7 +1270,7 @@ public class AnalyzeExprsTest extends AnalyzerTest { private void checkReturnType(String stmt, Type resultType) { SelectStmt select = (SelectStmt) AnalyzesOk(stmt); - ArrayList<Expr> selectListExprs = select.getResultExprs(); + List<Expr> selectListExprs = select.getResultExprs(); assertNotNull(selectListExprs); assertEquals(selectListExprs.size(), 1); // check the first expr in select list
