impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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:


Looks good overall. I had a few minor style comments then a bigger question about code-sharing
with Trunc()
File be/src/exprs/

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).
File be/src/exprs/

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.

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/
-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
File be/src/exprs/udf-builtins.h:

Line 99: 
Extra blank line.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message