Dan Hecht has posted comments on this change. Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: PS2, Line 637: FromUnixTime The problem is only with the functions that take sub-second resolution, right? I think that's important to clarify -- otherwise it's not clear what we mean by rounding. PS2, Line 685: n nit: typo 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()? That code looks bogus if date_.is_special(), so the call from ScalarColumnReader<TimestampValue, true>::ValidateSlot() seems dangerous (if the parquet file contained a corrupted value that corresponded to not_a_date_time). PS2, Line 81: ptime > There was no exception thrown at the time of creation, but much later, when We don't want to use exceptions in impala. We limit their use to when interacting with other libraries. So not throwing an exception here is the right thing. Line 86: } why is that that the year() calls in timestamp-functions-ir.cc aren't sufficient, e.g.: // Validate that the ptime is not "special" (ie not_a_date_time) and has a valid year. // If validation fails, an exception is thrown. datetime.date().year(); Centralizing where we do this validation seems like a good idea, but unlike the other code that attempts to do this kind of thing, e.g. TimestampFunctions::AddSub(), we don't get a warning. The user should probably get a warning in this case. 2) If we're going to do the validation in a central place, we should try to clean up the other code that does the year() calls (assuming it's trying to validate the same thing). -- 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
