impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "sandeep akinapelli (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5317: add DATE TRUNC() function
Date Sun, 27 Aug 2017 23:22:16 GMT
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 <sakinapelli@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sakinapelli@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message