Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708282230.v7SMUaO4030625@ip-10-146-233-104.ec2.internal>
Date: Mon, 28 Aug 2017 22:30:36 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: sandeep akinapelli <sakinapelli@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5317=3A_add_DATE_TRUNC=28=29_function=0A?=
In-Reply-To: <gerrit.1498625022000.I953ba006cbb166dcc78e8c0c12dfbf70f093b584@gerrit.cloudera.org>
References: <gerrit.1498625022000.I953ba006cbb166dcc78e8c0c12dfbf70f093b584@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 28 Aug 2017 22:30:42 -0000

Tim Armstrong has posted comments on this change.

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


Patch Set 2:

(15 comments)

Just minor comments, mainly cleanup. I'd also like someone else to look over the code, particularly the tests, to make sure we're not missing any edge cases (we've been burned in the past with subtle timestamp code).

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

Line 7392:   TestIsNull("date_trunc('MILLENNIUM', '1416-05-08 10:30:00')", TYPE_TIMESTAMP);
It would be good to test MILLENIUM with 1999-12-31 11:59:59 or similar since there's a special case in the code for 2000.


Line 7393:   TestIsNull("date_trunc('WEEK', '1400-01-01 00:00:00')", TYPE_TIMESTAMP);
I think we should also test the edge cases 1400-1-6 and 1400-1-7 to make sure there isn't an off-by-one error in the calculation.


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

Line 420: 
> Yes agree with you. MY thoughts initially were that if there are conflicts 
Yeah I agree, I went down the same line of reasoning but I think this works out better.


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?


Line 245:   // fractional seconds are nanoseconds as per the build configuration
This comment is a bit cryptic - not clear which build configuration it means. Maybe something like "Fractional seconds are nanoseconds because Boost is configured to use nanosecond precision".


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


Line 260: // used by both Trunc and DateTrunc functions to perform the truncation
Put doTrunc() in an anonymous namespace -  "namespace {" - to avoid avoid exporting the symbol from this module.


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


Line 267:   static const date week_limit_date(1400, 1, 6);
local static variables have weird semantics - it would be best to avoid them. How about just inlining the value on line 279 - the value is already mentioned in the comment.


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


Line 275:       if (orig_date.is_special()) return TimestampVal::null();
Shouldn't we check whether the date is special before checking the year? I'm not sure that the year is valid otherwise.


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


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


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


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


-- 
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