-
Notifications
You must be signed in to change notification settings - Fork 750
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
add checker for appending i128 to decimal builder #928
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @liukun4515
If you can please avoid adding a new variant to ArrowError
I think this PR is ready to merge.
arrow/src/error.rs
Outdated
@@ -41,6 +41,7 @@ pub enum ArrowError { | |||
/// Error during import or export to/from the C Data Interface | |||
CDataInterface(String), | |||
DictionaryKeyOverflowError, | |||
DecimalError(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new variant to this enum
makes the change, strictly speaking, not backwards compatible.
I think we should use InvalidArgumentError
rather than adding a new variant in order to be consistent with the rest of the codebase as well as ensure this change is backwards compatible (and can be included in the 6.2.0 release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your comments. I will address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb I have updated this pull request according to your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @liukun4515 👍
* add check for appending i128 to decimal builder * remove the ArrowError(DecimalError)
Which issue does this PR close?
Closes #927
Rationale for this change
the code doesn't check the value of input i128 for the decimalbuilder.
resolve the issue #927
What changes are included in this PR?
Add checker for the
append_value
function in theDecimalBuilder
.Are there any user-facing changes?
No