Jim Apple has posted comments on this change. Change subject: IMPALA-3504: UDF for current timestamp in UTC ......................................................................
Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/7203/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 259: vector<string> GetValues(const vector<string>& exprs, This is overwrought for a function that gets used once with a single ColumnType. Can you simplify it? PS1, Line 259: GetValues Give new methods comments explaining what they are for. PS1, Line 260: false is this just dead code - nobody calls this with expect_error = true, yes? Line 261: DCHECK(exprs.size() == expr_types.size()); !empty(), too, right? Line 264: for (const string expr : exprs) { One must be very careful with for loops like this. THis copies the string. Try 'const auto&'. Line 267: string stmt = ss.str(); const Line 268: stmt.erase(stmt.size() - 1); Just don't write the comma in the first place. Line 289: for (const ColumnType expr_type : expr_types) { const reference unless you want the copy. Line 290: EXPECT_EQ(TypeToOdbcString(expr_type.type), result_types[index].type) Move this up before the FetchResult Line 291: << exprs.at(index++); If you use at for indexing, use it consistently (see line above) Line 5210: TimestampValue utc_start_time = TimestampValue::UtcTime(); const PS1, Line 5226: TimestampValue const PS1, Line 5226: ConvertValue<TimestampValue> This could be in GetValues if this is the only call. Line 5229: EXPECT_TRUE(utc_timestamp == now); EXPECT_EQ? http://gerrit.cloudera.org:8080/#/c/7203/1/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 81: utc_timestamp_(new TimestampValue(TimestampValue::Parse(query_state->query_ctx().utc_timestamp_string))), long line - brak at or before 90. -- To view, visit http://gerrit.cloudera.org:8080/7203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I969fc805922f2bb9c8101e84f85ff2cc3b1b6729 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
