Tim Armstrong has posted comments on this change. Change subject: IMPALA-5317: add DATE_TRUNC() function ......................................................................
Patch Set 2: (15 comments) Just minor comments, mainly cleanup. I'd also like someone else to look over the code, particularly the tests, to make sure we're not missing any edge cases (we've been burned in the past with subtle timestamp code). http://gerrit.cloudera.org:8080/#/c/7313/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 7392: TestIsNull("date_trunc('MILLENNIUM', '1416-05-08 10:30:00')", TYPE_TIMESTAMP); It would be good to test MILLENIUM with 1999-12-31 11:59:59 or similar since there's a special case in the code for 2000. Line 7393: TestIsNull("date_trunc('WEEK', '1400-01-01 00:00:00')", TYPE_TIMESTAMP); I think we should also test the edge cases 1400-1-6 and 1400-1-7 to make sure there isn't an off-by-one error in the calculation. http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/udf-builtins.cc File be/src/exprs/udf-builtins.cc: Line 420: > Yes agree with you. MY thoughts initially were that if there are conflicts Yeah I agree, I went down the same line of reasoning but I think this works out better. http://gerrit.cloudera.org:8080/#/c/7313/2/be/src/exprs/udf-builtins.cc File be/src/exprs/udf-builtins.cc: PS2, Line 52: untils units? Line 245: // fractional seconds are nanoseconds as per the build configuration This comment is a bit cryptic - not clear which build configuration it means. Maybe something like "Fractional seconds are nanoseconds because Boost is configured to use nanosecond precision". Line 254: // fractional seconds are nanoseconds as per the build configuration. Same here. Line 260: // used by both Trunc and DateTrunc functions to perform the truncation Put doTrunc() in an anonymous namespace - "namespace {" - to avoid avoid exporting the symbol from this module. PS2, Line 261: doTrunc nit: casing should be DoTrunc(). Line 267: static const date week_limit_date(1400, 1, 6); local static variables have weird semantics - it would be best to avoid them. How about just inlining the value on line 279 - the value is already mentioned in the comment. PS2, Line 272: // for millenium < 2000 year value goes to 1000 : // (outside impala supported range) nit: comment fits on one line. Line 275: if (orig_date.is_special()) return TimestampVal::null(); Shouldn't we check whether the date is special before checking the year? I'm not sure that the year is valid otherwise. Line 280: if (orig_date.is_special()) return TimestampVal::null(); Same here - should we check special first? Line 289: // used by DateTrunc only This comment seems unnecessary (it's documented in the enum). Line 296: // used by DateTrunc only Same here. Line 334: // used by DateTrunc only Same here. -- To view, visit http://gerrit.cloudera.org:8080/7313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: sandeep akinapelli <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: sandeep akinapelli <[email protected]> Gerrit-HasComments: Yes
