Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-3163: Fix Decimal to Timestamp casting ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/3154/2/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: Line 569: int64_t > should these be int32_t in this case? if not, why? Yeah, it makes sense that they can be int32_t. The entire Decimal4Value is can fit into int32_t. Line 571: LIKELY > why is this LIKELY? I think it's going to be very rare that someone has a decimal with scale larger than 9 that they want to convert to timestamp. In other words, why would someone have a decimal column that represents timestamp with scale greater than 9 (for example picosecond precision)? Line 577: } > How about factoring this block into a (templated) subroutine? Done http://gerrit.cloudera.org:8080/#/c/3154/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1360: 260000000000 > what i'm asking is whether the upper limit changes between the old and new I did some investigating, but it seems it's very close, but not exactly the same. The larger the precision and scale, the bigger the difference is. I'll investigate some more tomorrow. Line 3766: We get some decimal-binary skew here > isn't this no longer the case? Done Line 3799: // We get some decimal-binary skew here > same Done -- 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: 2 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
