Skip to content

Commit 2083a10

Browse files
authored
refactor: DecimalSumState overflow handling in SUM aggregation (#17773)
fix: DecimalSumState overflow handling in SUM aggregation Fix the boolean logic in the implementation to correctly handle decimal overflow checking in aggregate SUM operations. Rename the generic parameter from OVERFLOW to SHOULD_CHECK_OVERFLOW to better reflect its purpose.
1 parent fb60eaf commit 2083a10

File tree

1 file changed

+14
-13
lines changed

1 file changed

+14
-13
lines changed

src/query/functions/src/aggregates/aggregate_sum.rs

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ where
165165
}
166166

167167
#[derive(BorshDeserialize, BorshSerialize)]
168-
pub struct DecimalSumState<const OVERFLOW: bool, T>
168+
pub struct DecimalSumState<const SHOULD_CHECK_OVERFLOW: bool, T>
169169
where
170170
T: ValueType,
171171
T::Scalar: Decimal,
@@ -174,7 +174,7 @@ where
174174
pub value: <T::Scalar as Decimal>::U64Array,
175175
}
176176

177-
impl<const OVERFLOW: bool, T> Default for DecimalSumState<OVERFLOW, T>
177+
impl<const SHOULD_CHECK_OVERFLOW: bool, T> Default for DecimalSumState<SHOULD_CHECK_OVERFLOW, T>
178178
where
179179
T: ValueType,
180180
T::Scalar: Decimal + std::ops::AddAssign,
@@ -187,7 +187,8 @@ where
187187
}
188188
}
189189

190-
impl<const OVERFLOW: bool, T> UnaryState<T, T> for DecimalSumState<OVERFLOW, T>
190+
impl<const SHOULD_CHECK_OVERFLOW: bool, T> UnaryState<T, T>
191+
for DecimalSumState<SHOULD_CHECK_OVERFLOW, T>
191192
where
192193
T: ValueType,
193194
T::Scalar: Decimal + std::ops::AddAssign,
@@ -201,7 +202,7 @@ where
201202
let mut value = T::Scalar::from_u64_array(self.value);
202203
value += T::to_owned_scalar(other);
203204

204-
if OVERFLOW && (value > T::Scalar::MAX || value < T::Scalar::MIN) {
205+
if SHOULD_CHECK_OVERFLOW && (value > T::Scalar::MAX || value < T::Scalar::MIN) {
205206
return Err(ErrorCode::Overflow(format!(
206207
"Decimal overflow: {:?} not in [{}, {}]",
207208
self.value,
@@ -219,7 +220,7 @@ where
219220
validity: Option<&Bitmap>,
220221
function_data: Option<&dyn FunctionData>,
221222
) -> Result<()> {
222-
if !OVERFLOW {
223+
if !SHOULD_CHECK_OVERFLOW {
223224
let mut sum = T::Scalar::from_u64_array(self.value);
224225
let col = T::upcast_column(other);
225226
let buffer = DecimalType::<T::Scalar>::try_downcast_column(&col).unwrap();
@@ -362,20 +363,20 @@ pub fn try_create_aggregate_sum_function(
362363
};
363364

364365
// DecimalWidth<int64_t> = 18
365-
let overflow = s.precision > 18;
366+
let should_check_overflow = s.precision > 18;
366367
let return_type = DataType::Decimal(DecimalDataType::from_size(decimal_size)?);
367368

368-
if overflow {
369+
if should_check_overflow {
369370
AggregateUnaryFunction::<
370-
DecimalSumState<false, Decimal128Type>,
371+
DecimalSumState<true, Decimal128Type>,
371372
Decimal128Type,
372373
Decimal128Type,
373374
>::try_create_unary(
374375
display_name, return_type, params, arguments[0].clone()
375376
)
376377
} else {
377378
AggregateUnaryFunction::<
378-
DecimalSumState<true, Decimal128Type>,
379+
DecimalSumState<false, Decimal128Type>,
379380
Decimal128Type,
380381
Decimal128Type,
381382
>::try_create_unary(
@@ -399,20 +400,20 @@ pub fn try_create_aggregate_sum_function(
399400
scale: s.scale,
400401
};
401402

402-
let overflow = s.precision > 18;
403+
let should_check_overflow = s.precision > 18;
403404
let return_type = DataType::Decimal(DecimalDataType::from_size(decimal_size)?);
404405

405-
if overflow {
406+
if should_check_overflow {
406407
AggregateUnaryFunction::<
407-
DecimalSumState<false, Decimal256Type>,
408+
DecimalSumState<true, Decimal256Type>,
408409
Decimal256Type,
409410
Decimal256Type,
410411
>::try_create_unary(
411412
display_name, return_type, params, arguments[0].clone()
412413
)
413414
} else {
414415
AggregateUnaryFunction::<
415-
DecimalSumState<true, Decimal256Type>,
416+
DecimalSumState<false, Decimal256Type>,
416417
Decimal256Type,
417418
Decimal256Type,
418419
>::try_create_unary(

0 commit comments

Comments
 (0)