[REVIEW] Clip decimal binary op precision at max precision

[ChrisJar]

This fixes an issue where the precision assigned to the result of a decimal binary operation may exceed the maximum precision.

Closes #8291

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.06@bbce6bc). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.06    #8194   +/-   ##
===============================================
  Coverage                ?   82.84%           
===============================================
  Files                   ?      105           
  Lines                   ?    17866           
  Branches                ?        0           
===============================================
  Hits                    ?    14801           
  Misses                  ?     3065           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbce6bc...9ed25ce. Read the comment docs.

[isVoid]

Can we add to the docstring of _binop_precision mentioning that this helper function does not guarantee result to be bounded by cudf.Decimal64Dtype.MAX_PRECISION?

[cwharris]

lgtm, but have a question.

[isVoid]

Sorry about the back and forth. Just a small comment reverting my previous review.

[cwharris]

Does this PR close an issue, and if so can you reference it here?

[ChrisJar]

Does this PR close an issue, and if so can you reference it here?

Nope, there's no issue related to this problem

[cwharris]

@ChrisJar In that case is there more context we can add to this PR? Adding some tests that would have previously failed, and/or providing the error that was being encountered previously could prove useful if we end up with a regression later on.

[ChrisJar]

@ChrisJar In that case is there more context we can add to this PR? Adding some tests that would have previously failed, and/or providing the error that was being encountered previously could prove useful if we end up with a regression later on.

Yep! Will do

[isVoid]

Thanks to @cwharris 's question that prompted me to think deeper: since we compute the precisions pessimistically, when clipping happens, it means the operation might have produced an overflow. Do we need to introspect the data to tell if the overflow has happened? What is current behavior when overflow happens?

[ChrisJar]

Yep good point. When the underlying int value of the result exceeds MAX_INT64 it overflows. However if it exceeds 999,999,999,999,999,999, but is less than MAX_INT64 it will produce a value with a precision of 19.

[harrism]

Yep good point. When the underlying int value of the result exceeds MAX_INT64 it overflows. However if it exceeds 999,999,999,999,999,999, but is less than MAX_INT64 it will produce a value with a precision of 19.

So are changes needed on this PR, or is it ready to merge?

[ChrisJar]

@harrism Yep! It's ready to merge

[harrism]

@gpucibot merge