Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/7954 )
Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7954/5/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/7954/5/be/src/runtime/timestamp-value.h@173 PS5, Line 173: This became static to enable other functions to check whether their date is valid or not without actually creating a TimestampValue or calling year() to generate an exception. I think it would be better if there was a single function that does this check, so if 1400 will be changed for some reason ( IMPALA-2009 ), there will be no need to change many parts of the code. http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h@86 PS2, Line 86: } > One purpose of FunctionContext is to be the interface back into impala's ru I think it would be better to do the logging on the caller side, because depending on the caller, the format for the log can be different, e.g. unix timestamp vs YYYY-MM-DD string. -- To view, visit http://gerrit.cloudera.org:8080/7954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff Gerrit-Change-Number: 7954 Gerrit-PatchSet: 2 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-Comment-Date: Thu, 21 Sep 2017 17:29:05 +0000 Gerrit-HasComments: Yes
