impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5317: add DATE TRUNC() function
Date Thu, 06 Jul 2017 21:24:38 GMT
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 <sakinapelli@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message