Dan Hecht has posted comments on this change. Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exec/data-source-scan-node.cc File be/src/exec/data-source-scan-node.cc: PS1, Line 284: FromUnixTimeNanos does it make sense that this conversion is subjected to use local tz flag? in any case, okay to leave this change as purely non-functional and we can deal with this later. http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: PS1, Line 354: TimestampValue maybe use "const TimetampValue& tv = ..." here (and elsewhere), though maybe unnecessary if the compiler is able to optimize away the copy anyway. http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: PS1, Line 81: lexical_cast<string>(t) how about getting rid of this implicit code as well by defining a ToString() (or maybe rename DebugString() to ToString())? http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: PS1, Line 619: heh PS1, Line 632: coverted converted PS1, Line 641: EXPECT_TRUE(leap_tv1.ToUnixTime(&leap_time_t)); : EXPECT_EQ(915148800, leap_time_t); : // Converted to the Unix time representation : EXPECT_EQ("1999-01-01 00:00:00", leap_tv1.DebugString()); maybe do these cases on leap_ptime2 as well, just to be sure everything is really the same. http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: PS1, Line 91: Conversion to local time will be done if Return the corresponding timestamp in the local timezone if .... PS1, Line 92: . Otherwise, return the corresponding timestamp in UTC. PS1, Line 97: 'FromUnixTime' For functions, we usually write: FromUnixTime() PS1, Line 105: unix_time 'unix_time' PS1, Line 287: operator do we need this other than that lexical_cast I commented on? -- To view, visit http://gerrit.cloudera.org:8080/6510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-HasComments: Yes
