Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201702242206.v1OM6DTM017901@ip-10-146-233-104.ec2.internal>
Date: Fri, 24 Feb 2017 22:06:13 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Zach Amsden <zamsden@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: dhecht@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: Fri, 24 Feb 2017 22:06:24 -0000

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4813: Round on divide and multiply
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

PS2, Line 682: ROUND
> Multiply gets defined using this macro, and multiply needs round.  Otherwis
GetConstFnAttr() is just as free as a constant 'false'.  The call goes away and is replaced by a constant when generating the code.


http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.h
File be/src/runtime/decimal-value.h:

Line 124:       int result_scale, bool* overflow) const {
> Testing code, called from be/src/runtime/decimal-test.cc, which forges its 
If they are only used by tests, I agree killing them would be nice.


http://gerrit.cloudera.org:8080/#/c/6132/2/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

Line 153:   DCHECK_EQ(round, false);
> I'm not opposed to removing these.  I inserted them mostly to make sure I g
but what i'm saying is we should actually pass the correct value of 'round', so the dcheck isn't valid. It happens that given our current result type choices, rounding for these requires no changes.


-- 
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: 2
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