Dan Hecht has posted comments on this change. Change subject: IMPALA-3163: Fix Decimal to Timestamp casting ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/3154/5/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: Line 581: const uint64_t LARGEST_EPOCH = 253402300799; Let's not expose this timestamp detail outside of timestamp. If we want to fix this bug, it should be done inside the TimestampValue class itself so that the behavior is consistent across all cases (e.g. convert from integer). But, I think that fix should be done as a separate patch. For the decimal specific problem, we should just check the int128_t case for overflow (and you need to check for negative values as well). So something like this in the case 16 block: if (seconds > numeric_limit<int64_t>::max() || seconds < numeric_limit<int64_t>::min()) { // TimeStampVal() takes int64_t. return null } Please have test cases at int64_t max()+1, max(), min(), min()-1 as well. http://gerrit.cloudera.org:8080/#/c/3154/5/be/src/exprs/decimal-operators.h File be/src/exprs/decimal-operators.h: Line 150: int target_scale, const DecimalRoundOp& op); add a blank line here Line 151: /// Returns the fractional_part in nanosecond scale. Document the scale parameter. Maybe something like: Coverts 'fraction_part' with the given 'scale' to nanoseconds. You could also rename it ConvertToNanoseconds() since it doesn't really have to be tied to the Timestamp datatype, but okay to leave the current name if you prefer. -- To view, visit http://gerrit.cloudera.org:8080/3154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabeea9f4ab4880b2f814408add63c77916e2dba9 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
