Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201702280136.v1S1aBn7027223@ip-10-146-233-104.ec2.internal>
Date: Tue, 28 Feb 2017 01:36:11 +0000
From: "Zach Amsden (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>
Reply-To: zamsden@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4813=3A_Round_on_divide_and_multiply=0A?=
In-Reply-To: <gerrit.1487898092000.Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312@gerrit.cloudera.org>
References: <gerrit.1487898092000.Ie6bfcbe37555b74598d409c6f84f06b0ae5c4312@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Tue, 28 Feb 2017 01:36:19 -0000

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 <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes