Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase output scale for decimal division #4640

Closed

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Aug 3, 2023

Which issue does this PR close?

Closes #.

Rationale for this change

apache/datafusion#6828

What changes are included in this PR?

Are there any user-facing changes?

Honestly not sure, this does change the output types for some decimal arithmetic, whether that counts as a breaking change is potentially debatable given this is not something documented, and regardless is a very recent feature addition.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 3, 2023
@westonpace
Copy link
Member

This covers the first part of the rules but will quickly reach overflow if you combine operations without using the second rules to reduce scale:

  private static Decimal adjustScaleIfNeeded(int precision, int scale) {
    if (precision > MAX_PRECISION) {
      int minScale = Math.min(scale, MIN_ADJUSTED_SCALE);
      int delta = precision - MAX_PRECISION;
      precision = MAX_PRECISION;
      scale = Math.max(scale - delta, minScale);
    }
    return new Decimal(precision, scale, 128);
  }

@tustvold tustvold marked this pull request as draft August 4, 2023 18:41
@tustvold
Copy link
Contributor Author

tustvold commented Aug 5, 2023

@westonpace I'm having a hard time following the logic behind adjustScaleIfNeeded. I can understand truncating the scale of the output in order to make it fit, however, the MIN_ADJUSTED_SCALE part doesn't make sense to me. It acts to clamp the scale to be <= 6, but without this having an impact on the output precision, which seems to be incorrect?

Say the output had a scale of 10 and a precision of 40, adjustScaleIfNeeded would then clamp this to a scale of 6 and a precision of 38. I would have thought it would either clamp to a scale of 8 and precision of 38, or a scale of 6 and a precision of 36?

Edit: oh no nvm, brain fart. It exists for the case of a scale of 5 and a precision of 40, it will refuse to reduce the scale further. This does mean that such an operation could still overflow though.

@tustvold
Copy link
Contributor Author

tustvold commented Aug 7, 2023

I've filed #4664 and have a preliminary implementation of what it would take to support precision-loss decimal arithmetic. I hope to polish this up over the coming week

@tustvold tustvold closed this Aug 12, 2023
@tustvold
Copy link
Contributor Author

Decided to not move forward with this, see #4664 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants