sandeep akinapelli has posted comments on this change. Change subject: IMPALA-5317: add DATE_TRUNC() function ......................................................................
Patch Set 2: (7 comments) addressed review comments. http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 7307: TimestampValue::Parse("2000-01-01 00:00:00 ")); > Extra blank line Done Line 7311: TimestampValue::Parse("2110-01-01 00:00:00")); > Nit should indent continuations by 4 extra spaces. Done Line 7355: TestTimestampValue("date_trunc('WEEK', '9999-12-31 23:59:59.999999999')", > Extra blank line Done Line 7358: TimestampValue::Parse("9999-12-31 00:00:00")); > I think it would be clearer to separate the test cases where the input is v Done http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/udf-builtins.cc File be/src/exprs/udf-builtins.cc: Line 56: WEEK, > Let's convert this and TruncUnit to use the C++11 "strongly typed enum" fea Removed the DateTruncUnit enum altogether and made the existing TruncUnit strongly typed enum. Line 99: } else if (unit == "hour") { > else if goes on the same line. https://cwiki.apache.org/confluence/pages/vi Done Line 420: > There's a lot of code duplication with Trunc(). Yes agree with you. MY thoughts initially were that if there are conflicts with how we interpret the units in the implementation, then it is murky. However thinking again, I dont think the definition of these functions will change over time. hence merged the code excpet the StrtoTruncunit. -- 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: Tim Armstrong <[email protected]> Gerrit-Reviewer: sandeep akinapelli <[email protected]> Gerrit-HasComments: Yes
