Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201707270103.v6R13plh017986@ip-10-146-233-104.ec2.internal>
Date: Thu, 27 Jul 2017 01:03:51 +0000
From: "Zach Amsden (Code Review)" <gerrit@cloudera.org>
To: Taras Bobrovytsky <tbobrovytsky@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>, Tim Armstrong <tarmstrong@cloudera.com>,
        Michael Ho <kwho@cloudera.com>
Reply-To: zamsden@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4939=2C_IMPALA-4939=3A_Decimal_V2_multiplication=0A?=
In-Reply-To: <gerrit.1500075819000.I37ad6232d7953bd75c18dc86e665b2b501a1ebe1@gerrit.cloudera.org>
References: <gerrit.1500075819000.I37ad6232d7953bd75c18dc86e665b2b501a1ebe1@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: Thu, 27 Jul 2017 01:03:58 -0000

Zach Amsden has posted comments on this change.

Change subject: IMPALA-4939, IMPALA-4939: Decimal V2 multiplication
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7438/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1554: }
> I think we aren't using 128 bit literals in that many places in our codebas
Agreed, this is fine for now.


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

Line 301:     needs_int256 = DecimalUtil::MAX_UNSCALED_DECIMAL16 / abs(y) < abs(x);
> Are you talking about using some existing special functions for counting th
A while loop will perform terribly, but there are intrinsics that can be used to count the zeros efficiently.  If we do that, this division can be optimized away if we note the number of zeros in the absolute value of the multiplicands is greater than 128.

We still need to check for a value > MAX_UNSCALED_DECIMAL16, but I think this could be a win over the division.

Maybe just adding a note or TODO for this for now.


http://gerrit.cloudera.org:8080/#/c/7438/1/be/src/util/bit-util.h
File be/src/util/bit-util.h:

Line 65:     return value < 0 ? -1 : 1;
> The way it was written before didn't work with int256, so I made this chang
I think we want to think carefully about using the Sign function with int256.  That boost type is likely to do something crazy here no matter what we code.  It's also always signed, and shifting it is a really bad idea.  Doing anything with it at all aside from the multiplication is going to be less than optimal.

Can we cheat and round in 128 bit precision instead?

I.e...
  int256 prod = x * y;
  result = prod / scale_multiplier;
  remainder = prod % scale_multiplier;
  // scale multiplier is always a power of 2, so
  if (remainder > scale_multiplier >> 1) result = 
      RoundAwayFromZero(result)

If we can get both remainder and quotient together without added cost from Boost, that is.

Worth running a simple benchmark to test, how about using TPC-DS and running:

 sum(l_quantity * l_tax) | sum(l_extendedprice * l_discount)


http://gerrit.cloudera.org:8080/#/c/7438/1/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

Line 336:   if (LIKELY(scale < 77)) return values[scale];
> It is possible to get large values, but when we are Dividing, not multiplyi
Really?  That's crazy.

Can we actually kill GetScaleMultiplier then?


http://gerrit.cloudera.org:8080/#/c/7438/1/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java:

Line 260:         resultPrecision = p1 + p2 + 1;
> I agree that the +1 is messy, but to be compatible with other systems and f
I think the 0.999 * 0.999 case is a fluke - we are sacrificing precision for every single other maximum precision case just for the 1e-74 chance that this one case rounds nicely.

I'd strongly prefer to see the crazy +1 go the way of the dodo, understand the compatibility argument, but wonder how important that actually is.


-- 
To view, visit http://gerrit.cloudera.org:8080/7438
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes