alamb commented on code in PR #11084:
URL: https://github.com/apache/datafusion/pull/11084#discussion_r1650272446
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -5494,6 +5566,69 @@ mod tests {
Ok(())
}
+ #[test]
+ #[allow(arithmetic_overflow)] // we want to test them
+ fn test_scalar_negative_overflows() -> Result<()> {
+ macro_rules! test_overflow_on_value {
+ ($($val:expr),* $(,)?) => {$(
+ {
+ let value: ScalarValue = $val;
+ let err = value.arithmetic_negate().expect_err("Should
receive overflow error on negating {value:?}");
+ let root_err = err.find_root();
+ match root_err{
+ DataFusionError::ArrowError(
+ ArrowError::ComputeError(_),
+ _,
+ ) => {}
+ _ => return Err(err),
+ };
+ }
+ )*};
+ }
+ test_overflow_on_value!(
+ // the integers
+ i8::MIN.into(),
+ i16::MIN.into(),
+ i32::MIN.into(),
+ i64::MIN.into(),
+ // for decimals, only value needs to be tested
+ ScalarValue::try_new_decimal128(i128::MIN, 10, 5)?,
+ ScalarValue::Decimal256(Some(i256::MIN), 20, 5),
+ // interval, check all possible values
+ ScalarValue::IntervalYearMonth(Some(i32::MIN)),
+ ScalarValue::new_interval_dt(i32::MIN, 999),
+ ScalarValue::new_interval_dt(1, i32::MIN),
+ ScalarValue::new_interval_mdn(i32::MIN, 15, 123_456),
+ ScalarValue::new_interval_mdn(12, i32::MIN, 123_456),
+ ScalarValue::new_interval_mdn(12, 15, i64::MIN),
+ // tz doesn't matter when negating
+ ScalarValue::TimestampSecond(Some(i64::MIN), None),
+ ScalarValue::TimestampMillisecond(Some(i64::MIN), None),
+ ScalarValue::TimestampMicrosecond(Some(i64::MIN), None),
+ ScalarValue::TimestampNanosecond(Some(i64::MIN), None),
+ );
+
+ let float_cases = [
+ (
+ ScalarValue::Float16(Some(f16::MIN)),
+ ScalarValue::Float16(Some(f16::MAX)),
+ ),
+ (
+ ScalarValue::Float16(Some(f16::MAX)),
+ ScalarValue::Float16(Some(f16::MIN)),
+ ),
+ (f32::MIN.into(), f32::MAX.into()),
+ (f32::MAX.into(), f32::MIN.into()),
+ (f64::MIN.into(), f64::MAX.into()),
+ (f64::MAX.into(), f64::MIN.into()),
+ ];
+ // skip float 16 because they aren't supported
+ for (test, expected) in float_cases.into_iter().skip(2) {
Review 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)
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1177,40 +1185,103 @@ impl ScalarValue {
| ScalarValue::Float64(None) => Ok(self.clone()),
ScalarValue::Float64(Some(v)) =>
Ok(ScalarValue::Float64(Some(-v))),
ScalarValue::Float32(Some(v)) =>
Ok(ScalarValue::Float32(Some(-v))),
- ScalarValue::Int8(Some(v)) => Ok(ScalarValue::Int8(Some(-v))),
- ScalarValue::Int16(Some(v)) => Ok(ScalarValue::Int16(Some(-v))),
- ScalarValue::Int32(Some(v)) => Ok(ScalarValue::Int32(Some(-v))),
- ScalarValue::Int64(Some(v)) => Ok(ScalarValue::Int64(Some(-v))),
+ ScalarValue::Int8(Some(v)) =>
Ok(ScalarValue::Int8(Some(v.neg_checked()?))),
+ ScalarValue::Int16(Some(v)) =>
Ok(ScalarValue::Int16(Some(v.neg_checked()?))),
+ ScalarValue::Int32(Some(v)) =>
Ok(ScalarValue::Int32(Some(v.neg_checked()?))),
+ ScalarValue::Int64(Some(v)) =>
Ok(ScalarValue::Int64(Some(v.neg_checked()?))),
ScalarValue::IntervalYearMonth(Some(v)) => {
- Ok(ScalarValue::IntervalYearMonth(Some(-v)))
+ Ok(ScalarValue::IntervalYearMonth(Some(neg_checked_with_ctx(
+ *v,
+ format!("In negation of IntervalYearMonth({v})"),
+ )?)))
}
ScalarValue::IntervalDayTime(Some(v)) => {
let (days, ms) = IntervalDayTimeType::to_parts(*v);
- let val = IntervalDayTimeType::make_value(-days, -ms);
+ let val = IntervalDayTimeType::make_value(
+ neg_checked_with_ctx(
+ days,
+ format!("In negation of days {days} in
IntervalDayTime"),
Review 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
```rust
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
