feat: [Variant] Add Validation for Variant Deciaml #7738
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Weijun-H
changed the title
Contributor
|
@scovich and @friendlymatthew perhaps you have some time to review this PR as well |
Weijun-H
force-pushed
the
7697-variant-decimal-validation
branch
2 times, most recently
from
681e4f1 to
5826ca6
Compare
Weijun-H
force-pushed
the
7697-variant-decimal-validation
branch
from
5826ca6 to
2ed858b
Compare
Weijun-H
force-pushed
the
7697-variant-decimal-validation
branch
from
ca8b76e to
e71f52f
Compare
alamb
changed the title
alamb
approved these changes
Jun 24, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Weijun-H -- this looks great to me
I am going to merge this one in so we minimize conflicts with @friendlymatthew 's PR
Contributor
|
I made a small follow on PR as well |
alamb
added a commit
to alamb/arrow-rs
that referenced
this pull request
Jun 24, 2025
carpecodeum
pushed a commit
to cmu-db/arrow-rs
that referenced
this pull request
Jun 25, 2025
alamb
added a commit
that referenced
this pull request
Jun 25, 2025
…ing tests (#7776) # Which issue does this PR close? * Relates to #7697 * Part of #6736 # Rationale for this change As a follow-up to #7738, we should verify that the unscaled integer value fits in the max precision (scale factor was already validated). # What changes are included in this PR? Add the missing checking, and add missing unit tests for both precision and scale. Also move the `VariantDecimalXX` structs to their own mod. # Are these changes tested? Yes, see above. # Are there any user-facing changes? No. Public re-rexport of the moved structs. Co-authored-by: Andrew Lamb <[email protected]>
alamb
added a commit
that referenced
this pull request
Jun 27, 2025
…es (#7770) # Which issue does this PR close? We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. - part of #6736 # Rationale for this change I noticed this while reviewing @Weijun-H's PR - #7738 I didn't know what part of the crate were using the fields directly and thus what part could potentially be creating invalid variants. By making the fields non `pub(crate)` I think it makes it clear these can not be constructed invalidly This also gave me a good excuse to write some more examples # What changes are included in this PR? 1. make fields in `VariantDecimal*` private 2. Add doc examples examples # Are these changes tested? By CI # Are there any user-facing changes? API change in a non-published crate, so no end user impact yet
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
VariantBuilder#7697Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?