Zach Amsden has posted comments on this change. Change subject: IMPALA-4813: Round on divide and multiply ......................................................................
Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/6132/4/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS4, Line 122: /* round */ false > this should pass 'round' Done http://gerrit.cloudera.org:8080/#/c/6132/4/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 194: namespace detail { > add a function comment. Done PS4, Line 198: be > garbled Done Line 205: // here that it is a power of two. > I think you mean "even" (or "power of ten"), not "power of two". Done Line 283: *overflow |= abs(result) > DecimalUtil::MAX_UNSCALED_DECIMAL16; > shouldn't this have the: Done PS4, Line 311: 2 * remainder > do we know this can't overflow? can you comment on that (and maybe add DCHE Yes, because the maximum value is at most the 128 bit int scaled up by 10**38 (in reality our scales can't get that high). I don't know of a way to do bitwise asserts with an int256_t, I'll see if it works. DCHECK_EQ is almost certain to throw up all over itself. Line 327: if (abs(2 * remainder) >= abs(y)) { This is where I'm worried about overflow. Patient and grueling testing has not been able to produce one yet. We should be able to compute a value that overflows into the sign bit when doubled, but to do so requires an actual value that should round away from zero. The compiler seems to be using an unsigned test here between the abs() comparisons, so despite the undefined behavior, it appears to still get the right answer. Still investigating this a bit more thoroughly. http://gerrit.cloudera.org:8080/#/c/6132/4/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: Line 37: EXPECT_EQ(BitUtil::UnsignedWidth<char>(), 7); > this is implementation dependent. maybe say "signed char" to remove that de Done Line 42: EXPECT_GT(BitUtil::UnsignedWidth<int256_t>(), 256); Oh, this was the test I mentioned. BitUtil::IncrementAwayFromZero is undefined for non compiler implemented types. Line 60: EXPECT_EQ(BitUtil::IncrementAwayFromZero<int128_t>(-200), -201); > what happens with the 256 bit type we use in decimal code? does this work? I had a test for that but it seems to have run away. It gives larger than 256 bit values because the implementation is non-compact (and also not two's complement, so increment away from zero won't have any possibility to work anyway). http://gerrit.cloudera.org:8080/#/c/6132/4/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java: Line 266: // } > please remove this from this diff. it should go in IMPALA-4940 change. (als Done -- To view, visit http://gerrit.cloudera.org:8080/6132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
