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 Tue, 29 Aug 2017 22:59:44 GMT
sandeep akinapelli has posted comments on this change.

Change subject: IMPALA-5317: add DATE_TRUNC() function
......................................................................


Patch Set 2:

(12 comments)

Addressed review comments.

http://gerrit.cloudera.org:8080/#/c/7313/2/be/src/exprs/udf-builtins.cc
File be/src/exprs/udf-builtins.cc:

PS2, Line 52: untils
> units?
Done


Line 245:   // fractional seconds are nanoseconds as per the build configuration
> This comment is a bit cryptic - not clear which build configuration it mean
Done


Line 254:   // fractional seconds are nanoseconds as per the build configuration.
> Same here.
Done


Line 260: // used by both Trunc and DateTrunc functions to perform the truncation
> Put doTrunc() in an anonymous namespace -  "namespace {" - to avoid avoid e
Done. Its already inside the the namespace {} block.


PS2, Line 261: doTrunc
> nit: casing should be DoTrunc().
Done


Line 267:   static const date week_limit_date(1400, 1, 6);
> local static variables have weird semantics - it would be best to avoid the
Done


PS2, Line 272:       // for millenium < 2000 year value goes to 1000
             :       // (outside impala supported range)
> nit: comment fits on one line.
Done


Line 275:       if (orig_date.is_special()) return TimestampVal::null();
> Shouldn't we check whether the date is special before checking the year? I'
Yes. you are correct. switched the ifs.


Line 280:       if (orig_date.is_special()) return TimestampVal::null();
> Same here - should we check special first?
Done


Line 289:     // used by DateTrunc only
> This comment seems unnecessary (it's documented in the enum).
Done


Line 296:     // used by DateTrunc only
> Same here.
Done


Line 334:     // used by DateTrunc only
> Same here.
Done


-- 
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: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sakinapelli@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message