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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
