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

Expose min/max values for Decimal128/256 and improve docs #6992

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 17, 2025

Which issue does this PR close?

Rationale for this change

We need to know the min/max values for Decimal128 and Decimal256 in Datafusion (see apache/datafusion#14126 from @waynexia )

Currently only the min/max values are exposed for Decimal128

Also it turns out that the exposed values is different than what is currently used, which is confusing

What changes are included in this PR?

  1. Expose min/max values for both Decimal128 and Decimal256
  2. Add documentation and examples
  3. Rename the pub(crate) constants to be consistent
  4. Deprecate the old (unused) pub constants

Screenshot 2025-01-17 at 4 37 39 PM

Are there any user-facing changes?

Yes, deprecated some constants and made others public

@alamb alamb marked this pull request as ready for review January 20, 2025 13:17
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @alamb

/// [`Decimal128`]: arrow_schema::DataType::Decimal128
#[deprecated(
since = "54.1.0",
note = "Use MAX_DECIMAL128_FOR_EACH_PRECISION (note indexes are different)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alamb
Copy link
Contributor Author

alamb commented Jan 21, 2025

Thank you for the review @waynexia

@alamb
Copy link
Contributor Author

alamb commented Jan 22, 2025

I will plan to merge this PR tomorrow unless anyone else would like time to comment

@tustvold tustvold merged commit d6fa078 into apache:main Jan 25, 2025
26 checks passed
@alamb alamb deleted the alamb/expose_limits branch January 26, 2025 10:52
@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label Jan 27, 2025
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 enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants