Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4939, IMPALA-4939: Decimal V2 multiplication ......................................................................
Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/7438/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1554: } > This is the second time a similar function has come up in code review. Wha I think we aren't using 128 bit literals in that many places in our codebase that it warrants bringing in a library (that would need testing, etc) or using user defined literals. I think the current solution (the above function) is good enough for this test file. If we actually do want to do this, it would require a relatively significant effort. I created IMPALA-5697 to track this. Line 1693: { true, 0, 38, 6 }}}, > Other test - 38 9's right of decimal * 38 9's right of decimal. If I am co Done http://gerrit.cloudera.org:8080/#/c/7438/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 240: if (needs_int256) { > UNLIKELY(needs_int256) Done Line 251: if (delta_scale > 38) { > Can we explicitly handle the LIKELY(delta_scale == 0) case first? Done Line 253: // decimal(38, 38). The result should be a decimal(38, 37), so > Is this what our new formula gives? It is slightly paradoxical, since it l 0.9999 * 0.9999 gives a 1.000 after rounding, so it makes sense for the result to be (38,37). Added a test case for it as you suggested. Line 301: int256_t remainder = x % y; > I think this can be optimized a bit. Why not use count of leading zeroes a Are you talking about using some existing special functions for counting the number of leading zeros or implementing these functions myself with a while loop? Are there existing functions for 128 bit integers? What makes you think that it will be faster than what we try to do currently? Another thing, with that approach, isn't it possible that needs_int256 is false, but the result of the multiplication is greater than MAX_UNSCALED_DECIMAL16? We would need to add another check for this case. Line 305: // converted y to a 256 bit value, and remainder must be less than y, so there > UNLIKELY Done Line 316: *overflow |= abs(r) > DecimalUtil::MAX_UNSCALED_DECIMAL16; > As above, handle likely case of delta_scale == 0 first Done. The duplicated code was removed because the #if 5 <= __GNUC__ branch never runs today and cannot be tested. It can be added back in the future. 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 reason it was written that way is I think this is undefined for unsigne The way it was written before didn't work with int256, so I made this change. Also, it's easier to understand the function the way it is written in my patch compared to before. 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]; > Do we really need values this high? Maybe truncate the scale earlier and a It is possible to get large values, but when we are Dividing, not multiplying. It turns out this function is not necessary because the function on line 58 would get called instead for int256, this is what happens in the division case today. Removed it. 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; > This +1 is messy for several reasons - first, it isn't necessary at all exc I agree that the +1 is messy, but to be compatible with other systems and for the 0.999 * 0.999 case I think it would make sense to keep it. -- 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: 2 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
