Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3504: UDF for current timestamp in UTC ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7203/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: PS2, Line 5192: "now()"; this message isn't going to be very clear later PS2, Line 5193: utc_timestamp( same PS2, Line 5201: now local_time PS2, Line 5202: TimestampValue can you make this const, then make a separate TimestampValue which is to be converted (not const), with a name like "local_time_converted", e.g. const TimestampValue utc_time... TimestampValue local_time_converted(utc_time); local_time_converted.UtcToLocal(); EXPECT_EQ(local_time, local_time_converted); then it's more clear what we're checking PS2, Line 5187: const string stmt = "select now(), utc_timestamp()"; : vector<FieldSchema> result_types; : Status status = executor_->Exec(stmt, &result_types); : EXPECT_TRUE(status.ok()) << "stmt: " << stmt << "\nerror: " << status.GetDetail(); : DCHECK(result_types.size() == 2); : EXPECT_EQ(TypeToOdbcString(TYPE_TIMESTAMP), result_types[0].type) << "now()"; : EXPECT_EQ(TypeToOdbcString(TYPE_TIMESTAMP), result_types[1].type) << "utc_timestamp()"; : string result_row; : status = executor_->FetchResult(&result_row); : EXPECT_TRUE(status.ok()) << "stmt: " << stmt << "\nerror: " << status.GetDetail(); : vector<string> result_cols; : boost::split(result_cols, result_row, boost::is_any_of("\t")); : // To ensure this fails if columns are not tab separated : DCHECK(result_cols.size() == 2); : const TimestampValue now = ConvertValue<TimestampValue>(result_cols[0]); : TimestampValue utc_timestamp = ConvertValue<TimestampValue>(result_cols[1]); : utc_timestamp.UtcToLocal(); : EXPECT_EQ(utc_timestamp, now); can you wrap this in a set of braces to (1) make the code associated with this case easily identified and (2) scope some of the variables that won't be needed later but have generic names. e.g. { const string stmt... } -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
