Csaba Ringhofer has posted comments on this change. Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: PS2, Line 80: !date_.is_special() > how about HasDate()? But also, should IsValidDate() be checking HasDate()? I looked at ScalarColumnReader<TimestampValue, true>::ValidateSlot(), and now I see that TimestampValue is created via reinterpret_cast on buffer pointers - this means that its constructor is skipped, so it is not possible to force validity in the public functions. >That code looks bogus if date_.is_special(), Special values lie outside the valid time range, so IsValidDate can only return true if is_special() is false. TimestampValue can have not_a_date_time in date_ or time_, it is only invalid, if both are not_a_date_time. I am not sure, what will/should happen in these cases. Line 86: } > why is that that the year() calls in timestamp-functions-ir.cc aren't suffi About giving a warning to the user: is it ok to log in low level classes like TimestampValue? AddSub calls FunctionContext::AddWarning, while TimestampValue`s functions do not receive FunctionContext arguments. If TimestampValue functions cannot log, then I will have to look for their callers and add checks + warnings there. Note that I am not familiar with Impala`s logging system yet. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
