[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash impala (boost exception)

[Lars Volker (Code Review)]

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, .12.31 59.59.59 is a bit different, because as I 
understand,
: with nanosecond precision posix times, the maximum value is 
actually
: 1.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 1<=.
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 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash impala (boost exception)

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7954

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

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

TimestampValue::FromSubsecondUnixTime and UtcFromUnixTimeMicros
are incorrect only in case of the last second of 1399, because these
sub-second values are rounded first towards 1400.01.01 00.00.00, which
is accepted as a valid date, and the sub-second part is subtracted
afterwards, leading to a date outside the valid interval. The maximum
case, .12.31 59.59.59 is a bit different, because as I understand,
with nanosecond precision posix times, the maximum value is actually
1.01.01. 00.00.00 minus 1 nanosec.

TimestampValue::FromUnixTimeNanos can create problematic TimestampValues
both <1400 and 1<=.

These timestamps can cause problems, because most code assumes that if
HasDate/HasTime is true, then it really is a valid timestamp.

Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
---
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.h
2 files changed, 10 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/7954/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer