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 <youwei.a.w...@intel.com>
Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <youwei.a.w...@intel.com>
Gerrit-HasComments: Yes

Reply via email to