Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-3163: Fix Decimal to Timestamp casting ......................................................................
Patch Set 1: -Code-Review (3 comments) http://gerrit.cloudera.org:8080/#/c/3154/1/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: Line 565: TIMESTAMP_PRECISION > TIMESTAMP_SCALE (to be consistent with decimal terminology, and reword the Done Line 574: * > oh, i guess what's happening here is that we always have either mult==1 or Yeah that's what I was thinking initially, but there way this way it's a few percent faster according to my benchmarks (because there is no branch). Initially the way I implemented this was to precompute and store the mult and div coefficients in thread local storage. Strangely that turned out to be slower than this implementation (probably because reading from that storage is slow). I think it makes sense to implement it with the if statement because it's more readable and easier to understand. Line 599: TimestampValue tv(whole_pt, fract_pt); > what is the effective (precision, scale) for TIMESTAMP? aren't there some d Well, we don't support years larger than 10,000, so the whole part can't be bigger than about 250 billion. So when we convert an integer or double to timestamp that's larger than that, it just becomes NULL. If the scale of the decimal is larger than 9 (nanosecond), I think it makes sense to just reduce it to 9 (that's what we're doing now) without issuing any warnings, I doubt anyone expects the precision be larger than that. -- 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: 1 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
