-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove redundant unwrap in ScalarValue::new_primitive
, return a Result
#7830
Conversation
291f741
to
6de516d
Compare
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.
This looks good to me -- while it is an API change I think it is a reasonable one. Thank you @maruschin
I noticed the comments also need to be updated but we could also do that as a follow on PR as well
@@ -744,13 +744,13 @@ impl ScalarValue { | |||
pub fn new_primitive<T: ArrowPrimitiveType>( | |||
a: Option<T::Native>, | |||
d: &DataType, | |||
) -> Self { | |||
) -> Result<Self> { |
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.
I think the comments in this function also need to be updated (the 'panic's section is no longer correct)
/// Create a [`ScalarValue`] with the provided value and datatype
///
/// # Panics
///
/// Panics if d is not compatible with T
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.
with_data_type can still panic, I personally don't see anything wrong with this FWIW, panic are a perfectly valid way to handle unexpected failure imo
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.
Wrong with what? As in it is fine if this function panics and there is no need to propagate the error?
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.
Its an exceptional case that would imply some sort of schema inconsistency within the running plan, so imo panicking is perfectly valid, but then so is returning an error. The broader point is the doc is still correct
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.
I understand the point about the panics -- thank you
I don't have a strong opinion either way with regards to propagating errors vs panic'ing
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.
ScalarValue::new_primitive
ScalarValue::new_primitive
ScalarValue::new_primitive
, return a Result
a7f55b8
to
5befee1
Compare
5befee1
to
40020c2
Compare
Thanks @maruschin |
Which issue does this PR close?
It's a part of #3313.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?