fix: Add overflow check to evaluate of sum decimal accumulator #1922
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1922 +/- ##
============================================
+ Coverage 56.12% 58.83% +2.70%
- Complexity 976 1141 +165
============================================
Files 119 130 +11
Lines 11743 12853 +1110
Branches 2251 2393 +142
============================================
+ Hits 6591 7562 +971
- Misses 4012 4073 +61
- Partials 1140 1218 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The CI test failure is unrelated to changes in this PR and is now fixed in main branch |
| @@ -375,11 +375,17 @@ impl GroupsAccumulator for SumDecimalGroupsAccumulator { | |||
| // are null, in this case we'll return null | |||
| // 2. if `is_empty` is false, but `null_state` is true, it means there's an overflow. In | |||
| // non-ANSI mode Spark returns null. | |||
| let result = emit_to.take_needed(&mut self.sum); | |||
| result.iter().enumerate().for_each(|(i, &v)| { | |||
There was a problem hiding this comment.
In what case does this become necessary? decimal overflow is checked in update_batch (via update_single).
There was a problem hiding this comment.
Partial aggregate.
Use the test as example:
When 42 and 9*9 are parted.
part_1.update(42) // ok, 42
part_2.update(9*9) // ok, 9*9
// gather exchange
final.merge(part_1); final.merge(part_2) // overflow without setting nulls
final.evaluate() // overflowed value returned, spark throwBTW, when 42 and 9*9 are not parted, sum will work, but avg will fail, I am not yet put it to test though.
avg.update(42) // ok, 42
avg.update(9*9) // new_sum overflow, sum is still 42, count is 2 and null bit is set
avg.evaluate() // the evaluate of avg without checking the null bit, yielding 21There maybe other issues in avg, to be fixed later with #1893
There was a problem hiding this comment.
Makes sense. Thank you for the explanation!
leung-ming
changed the title
Which issue does this PR close?
Rationale for this change
summay overflow inmerge_batchWhat changes are included in this PR?
add overflow check to
evaluateHow are these changes tested?
org.apache.comet.exec.CometAggregateSuite