Jim Apple has posted comments on this change. Change subject: IMPALA-3504: function for current timestamp in UTC, i.e. utc_timestamp() ......................................................................
Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/4490/3//COMMIT_MSG Commit Message: Line 10: Generated timestamp has been verified using online UTC service Please try to break paragraphs close the red line, and put two line breaks at the end of a paragraph. PS3, Line 11: term of both epoch What do you mean by this? http://gerrit.cloudera.org:8080/#/c/4490/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 4117: timestamp_result = ConvertValue<TimestampValue>(GetValue("utc_timestamp()", Please check that this relates to now() in the way that you expect. http://gerrit.cloudera.org:8080/#/c/4490/3/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: Line 317: TimestampVal TimestampFunctions::Now(FunctionContext* context) { It looks to me like this is now only used one place. Can you inline it there, please, and take it out of the .h file? http://gerrit.cloudera.org:8080/#/c/4490/3/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS3, Line 121: now_utc 'utc_timestamp', here and below http://gerrit.cloudera.org:8080/#/c/4490/3/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: PS3, Line 113: universal time 'Coordinated Universal Time ("UTC")' PS3, Line 115: a daylight savings Does DST on the local machine change the universal time? PS3, Line 117: UniversalTime 'UtcTime' http://gerrit.cloudera.org:8080/#/c/4490/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 842: typedef boost::date_time::c_local_adjustor<ptime> local_adj; It's not a good use of a line to define a typedef that's used exactly once. PS3, Line 843: utime 'universal_time' PS3, Line 843: & Why are you using references, here and below? PS3, Line 844: ltime 'local_time' Line 845: query_ctx->__set_now_string(TimestampValue(ltime).DebugString()); DCHECK that ltime and utime are close - with 48 hours of each other, for instance. Line 846: query_ctx->__set_now_utc_string(TimestampValue(utime).DebugString()); If you set utime before ltime, either: 1. set the utime in query_ctx before the ltime in query_ctx; or 2. Set utime, then set utc_string in query_ctx, then ltime, then now_string in query_ctx. http://gerrit.cloudera.org:8080/#/c/4490/3/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS3, Line 298: now_utc_string 'utc_timestamp_string' -- To view, visit http://gerrit.cloudera.org:8080/4490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ee6ee192aa469f77c711f27ad324696a42004d1 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
