impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4939, IMPALA-4939: Decimal V2 multiplication
Date Mon, 24 Jul 2017 22:53:28 GMT
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 <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

Mime
View raw message