Tim Armstrong has posted comments on this change. Change subject: IMPALA-5317: add DATE_TRUNC() function ......................................................................
Patch Set 1: (8 comments) Looks good overall. I had a few minor style comments then a bigger question about code-sharing with Trunc() http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 7307: Extra blank line Line 7311: TimestampValue::Parse("9000-01-01 00:00:00")); Nit should indent continuations by 4 extra spaces. Line 7355: Extra blank line Line 7358: TestIsNull("date_trunc('MILLENNIUM', '1416-05-08 10:30:00')", TYPE_TIMESTAMP); I think it would be clearer to separate the test cases where the input is valid but the output is out-of-range (i.e. the first two). http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/udf-builtins.cc File be/src/exprs/udf-builtins.cc: Line 56: struct DateTruncUnit { Let's convert this and TruncUnit to use the C++11 "strongly typed enum" feature. I think this was written pre-C++11 in an attempt to emulate that. Line 99: } else if goes on the same line. https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 If you want to run clang-tidy to format your last commit you can run: git diff -U HEAD^ | $IMPALA_TOOLCHAIN/llvm-$IMPALA_LLVM_VERSION/share/clang/clang-format-diff.py -i -p1 Line 420: switch (date_trunc_unit) { There's a lot of code duplication with Trunc(). What do you think about combining the implementations? Was there a specific reason not to? I was thinking we could combine the sets of units (documenting those which are supported by Trunc()/ DateTrunc()/both) then merge the implementations except StrToDateTruncUnit() and StrToTruncUnit(). http://gerrit.cloudera.org:8080/#/c/7313/1/be/src/exprs/udf-builtins.h File be/src/exprs/udf-builtins.h: Line 99: Extra blank line. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: sandeep akinapelli <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
