impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash impala (boost exception)
Date Tue, 05 Sep 2017 20:13:11 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash impala (boost exception)
......................................................................


Patch Set 1:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/7954/1//COMMIT_MSG
Commit Message:

PS1, Line 7: impala
nit: Capitalize Impala (it's a name)


PS1, Line 9: FromSubsecondUnixTime
We usually refer to functions as Function().


PS1, Line 11: 1400.01.01 00.00.00
You could write dates in the more standard format (1400-01-01 00:00:00).


PS1, Line 13: The maximum
            : case, 9999.12.31 59.59.59 is a bit different, because as I understand,
            : with nanosecond precision posix times, the maximum value is actually
            : 10000.01.01. 00.00.00 minus 1 nanosec.
Can you add tests with sub-second deltas around the upper bound, too?


PS1, Line 18: TimestampValue::FromUnixTimeNanos can create problematic TimestampValues
            : both <1400 and 10000<=.
Doens't this change fix this?


Line 22: HasDate/HasTime is true, then it really is a valid timestamp.
Can you include in the commit message how you fixed it and how you test it? Generally speaking,
it's usually good to structure commit messages for bug fixes as "Problem, Solution, Tests".


http://gerrit.cloudera.org:8080/#/c/7954/1/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

Line 637:   // last second of 1399 need can be special, see https://issues.apache.org/jira/browse/IMPALA-5664
Please wrap lines at 90 characters. Please follow the surrounding code for indents and capitalization
of comments.


PS1, Line 637: need
Extra word?


PS1, Line 637: 1399
Please state of what.


PS1, Line 637: https://issues.apache.org/jira/browse/IMPALA-5664
Please outline in the comment briefly why the last second needs special attention.


Line 640:   EXPECT_FALSE(TimestampValue::FromUnixTimeNanos(MIN_DATE_AS_UNIX_TIME, -NANOS_PER_MICRO*100).HasDate());
While you are here, can you also add tests for the exact beginning of the last second?

We also should have tests that add a subsecond interval for symmetry.

Then please also add similar tests for the maximum date.


http://gerrit.cloudera.org:8080/#/c/7954/1/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS1, Line 80: !date_.is_special()
Can you add a comment explaining why this check is necessary?


Line 80: 	  if(UNLIKELY(!date_.is_special() && !IsValidDate())) *this = TimestampValue();
Please use spaces instead of tabs.


-- 
To view, visit http://gerrit.cloudera.org:8080/7954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message