Skip to content

Commit ed5f8e7

Browse files
authored
Precision::<usize>::{add, sub, multiply}: avoid overflows (#17929)
Use saturating operations Note this is not consistent with Precision::<ScalarValue> that wraps, happy to change behavior to wrapping here too
1 parent 7ce9b6e commit ed5f8e7

File tree

1 file changed

+43
-6
lines changed

1 file changed

+43
-6
lines changed

datafusion/common/src/stats.rs

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,15 @@ impl Precision<usize> {
120120
/// values is [`Precision::Absent`], the result is `Absent` too.
121121
pub fn add(&self, other: &Precision<usize>) -> Precision<usize> {
122122
match (self, other) {
123-
(Precision::Exact(a), Precision::Exact(b)) => Precision::Exact(a + b),
123+
(Precision::Exact(a), Precision::Exact(b)) => a.checked_add(*b).map_or_else(
124+
|| Precision::Inexact(a.saturating_add(*b)),
125+
Precision::Exact,
126+
),
124127
(Precision::Inexact(a), Precision::Exact(b))
125128
| (Precision::Exact(a), Precision::Inexact(b))
126-
| (Precision::Inexact(a), Precision::Inexact(b)) => Precision::Inexact(a + b),
129+
| (Precision::Inexact(a), Precision::Inexact(b)) => {
130+
Precision::Inexact(a.saturating_add(*b))
131+
}
127132
(_, _) => Precision::Absent,
128133
}
129134
}
@@ -133,10 +138,15 @@ impl Precision<usize> {
133138
/// values is [`Precision::Absent`], the result is `Absent` too.
134139
pub fn sub(&self, other: &Precision<usize>) -> Precision<usize> {
135140
match (self, other) {
136-
(Precision::Exact(a), Precision::Exact(b)) => Precision::Exact(a - b),
141+
(Precision::Exact(a), Precision::Exact(b)) => a.checked_sub(*b).map_or_else(
142+
|| Precision::Inexact(a.saturating_sub(*b)),
143+
Precision::Exact,
144+
),
137145
(Precision::Inexact(a), Precision::Exact(b))
138146
| (Precision::Exact(a), Precision::Inexact(b))
139-
| (Precision::Inexact(a), Precision::Inexact(b)) => Precision::Inexact(a - b),
147+
| (Precision::Inexact(a), Precision::Inexact(b)) => {
148+
Precision::Inexact(a.saturating_sub(*b))
149+
}
140150
(_, _) => Precision::Absent,
141151
}
142152
}
@@ -146,10 +156,15 @@ impl Precision<usize> {
146156
/// values is [`Precision::Absent`], the result is `Absent` too.
147157
pub fn multiply(&self, other: &Precision<usize>) -> Precision<usize> {
148158
match (self, other) {
149-
(Precision::Exact(a), Precision::Exact(b)) => Precision::Exact(a * b),
159+
(Precision::Exact(a), Precision::Exact(b)) => a.checked_mul(*b).map_or_else(
160+
|| Precision::Inexact(a.saturating_mul(*b)),
161+
Precision::Exact,
162+
),
150163
(Precision::Inexact(a), Precision::Exact(b))
151164
| (Precision::Exact(a), Precision::Inexact(b))
152-
| (Precision::Inexact(a), Precision::Inexact(b)) => Precision::Inexact(a * b),
165+
| (Precision::Inexact(a), Precision::Inexact(b)) => {
166+
Precision::Inexact(a.saturating_mul(*b))
167+
}
153168
(_, _) => Precision::Absent,
154169
}
155170
}
@@ -807,11 +822,21 @@ mod tests {
807822
let precision2 = Precision::Inexact(23);
808823
let precision3 = Precision::Exact(30);
809824
let absent_precision = Precision::Absent;
825+
let precision_max_exact = Precision::Exact(usize::MAX);
826+
let precision_max_inexact = Precision::Exact(usize::MAX);
810827

811828
assert_eq!(precision1.add(&precision2), Precision::Inexact(65));
812829
assert_eq!(precision1.add(&precision3), Precision::Exact(72));
813830
assert_eq!(precision2.add(&precision3), Precision::Inexact(53));
814831
assert_eq!(precision1.add(&absent_precision), Precision::Absent);
832+
assert_eq!(
833+
precision_max_exact.add(&precision1),
834+
Precision::Inexact(usize::MAX)
835+
);
836+
assert_eq!(
837+
precision_max_inexact.add(&precision1),
838+
Precision::Inexact(usize::MAX)
839+
);
815840
}
816841

817842
#[test]
@@ -843,6 +868,8 @@ mod tests {
843868

844869
assert_eq!(precision1.sub(&precision2), Precision::Inexact(19));
845870
assert_eq!(precision1.sub(&precision3), Precision::Exact(12));
871+
assert_eq!(precision2.sub(&precision1), Precision::Inexact(0));
872+
assert_eq!(precision3.sub(&precision1), Precision::Inexact(0));
846873
assert_eq!(precision1.sub(&absent_precision), Precision::Absent);
847874
}
848875

@@ -871,12 +898,22 @@ mod tests {
871898
let precision1 = Precision::Exact(6);
872899
let precision2 = Precision::Inexact(3);
873900
let precision3 = Precision::Exact(5);
901+
let precision_max_exact = Precision::Exact(usize::MAX);
902+
let precision_max_inexact = Precision::Exact(usize::MAX);
874903
let absent_precision = Precision::Absent;
875904

876905
assert_eq!(precision1.multiply(&precision2), Precision::Inexact(18));
877906
assert_eq!(precision1.multiply(&precision3), Precision::Exact(30));
878907
assert_eq!(precision2.multiply(&precision3), Precision::Inexact(15));
879908
assert_eq!(precision1.multiply(&absent_precision), Precision::Absent);
909+
assert_eq!(
910+
precision_max_exact.multiply(&precision1),
911+
Precision::Inexact(usize::MAX)
912+
);
913+
assert_eq!(
914+
precision_max_inexact.multiply(&precision1),
915+
Precision::Inexact(usize::MAX)
916+
);
880917
}
881918

882919
#[test]

0 commit comments

Comments
 (0)