Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors ......................................................................
Patch Set 1: (10 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? Hm I'm not sure, though if so I'll file a separate JIRA so it can be tracked externally as this would be a breaking change (which probably affects nearly nobody). 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 mayb Done 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( yeah this is weird, agreed http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: PS1, Line 632: coverted > converted Done 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 Done 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 .... Done PS1, Line 92: . > Otherwise, return the corresponding timestamp in UTC. Done PS1, Line 97: 'FromUnixTime' > For functions, we usually write: FromUnixTime() Done PS1, Line 105: unix_time > 'unix_time' Done PS1, Line 287: operator > do we need this other than that lexical_cast I commented on? I think we could work around it, but this does get used in a few places where we branch on the type and directly use this operator, e.g. literal.cc and raw-value.cc -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
