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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
