Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3504: UDF for current timestamp in UTC ......................................................................
Patch Set 1: (9 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 warrants a comment explaining what this does, the arguments, and the return value. Please mention what happens in the case of an error. PS1, Line 273: EXPECT_TRUE(expect_error) << "stmt: " << stmt << "\nerror: " : << status.GetDetail(); 1 line? I don't think this will be too long PS1, Line 283: EXPECT_TRUE(status.ok()) << "stmt: " << stmt << "\nerror: " : << status.GetDetail( 1 line? PS1, Line 291: index++) this is confusing here because it's not clear how this gets evaluated given the macro implementation. please do any potentially ambiguous var incrementing on a separate line. 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, please wrap http://gerrit.cloudera.org:8080/#/c/7203/1/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: Line 126: /// Returns Coordinated Universal Time ("UTC") with microsecond accuracy. This should Returns the current Coordinated Universal Time... http://gerrit.cloudera.org:8080/#/c/7203/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS1, Line 321: as at PS1, Line 371: as as -> at PS1, Line 371: (i nit: add space -- 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-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
