Dan Hecht has posted comments on this change. Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala ......................................................................
Patch Set 2: (3 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() > I looked at ScalarColumnReader<TimestampValue, true>::ValidateSlot(), and n My comment was just that HasDate() == !date_.is_special(), and using HasDate() would make it easier to grep all the places we check for this condition. I'm not sure I follow your comment about public functions. Since the scanner does a cast, it explicitly checks IsValidDate(). It is kinda gross, though. PS2, Line 80: !date_.is_special() > I think that ScalarColumnReader<TimestampValue, true>::ValidateSlot() is ac Thanks for finding that. Let's discuss on the jira. Line 86: } > About giving a warning to the user: is it ok to log in low level classes li One purpose of FunctionContext is to be the interface back into impala's runtime for UDFs (and expressions in general). It'd be best to leak it outside of exprs, udf, and codegen. So, I think it'd make sense to have a static wrapper around the constructor where we do this validation that lives in the exprs code, and that can have access to FunctionContext to do the logging, and construct the TimestampValue only for well formed values. (Then this constructor could do a dcheck for that). -- 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 <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-HasComments: Yes
