Overflow in negate operator#11084
Overflow in negate operator#11084alamb merged 6 commits intoapache:mainfrom LorrensP-2158466:overflow-in-negate-operator
Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @LorrensP-2158466 -- this looks good to me except for creating error messags on each row.
datafusion/common/src/scalar/mod.rs
Outdated
| let val = IntervalDayTimeType::make_value( | ||
| neg_checked_with_ctx( | ||
| days, | ||
| format!("In negation of days {days} in IntervalDayTime"), |
There was a problem hiding this comment.
I think that calling these functions like this will result in actually creating these error strings for each row, even if there is no error -- I think it is important to avoid that overhead.
I suspect it would be faster to use map_err like
let days = v.neg_checked()
.map_err(|e| {
DataFusionError::from(e)
.context(format!("In negation of milliseconds {ms} in IntervalDayTime")))
})?The reason being that in that formulation the format! is part of a closure that is only called when there is an actual error
There was a problem hiding this comment.
Yeah, I first tried that, but I didn't like repeating the same thing for every case. But this went over my head, I'll fix this back to map_err. Thanks for the tip!
| (f64::MAX.into(), f64::MIN.into()), | ||
| ]; | ||
| // skip float 16 because they aren't supported | ||
| for (test, expected) in float_cases.into_iter().skip(2) { |
There was a problem hiding this comment.
maybe we just shouldn't list float 16 at all 🤔 I think it might be more confusing to see them listed then skipped than simply not listed (or listed in a separate test that simply fails)
There was a problem hiding this comment.
Yeah I did this when filing #11083. Maybe it's cleaner to add a test that should fail on Float16 and when Float16 is supported, that test fails and the person making those changes knows to change the test.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
👨🍳 👌 @LorrensP-2158466 -- very nice. Thank you for this improvement
* Do checked negative op instead of unchecked * add tests for checking if overflow error occurs * add context to negating complexer ScalarValues * put format! call to create error message in closure * seperate test case for f16 that should panic with not implemented
2 participants
Which issue does this PR close?
Closes #11076. Tracking: #11078
Rationale for this change
What changes are included in this PR?
Doing checked negations on scalarValue
Are these changes tested?
With unit tests yes
Are there any user-facing changes?
no