Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2460][FUN] Fix sum() overflow bug ......................................................................
Patch Set 6: (2 comments) https://asterix-gerrit.ics.uci.edu/#/c/3012/6/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/AbstractSumAggregateFunction.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/AbstractSumAggregateFunction.java: Line 55: private double sum; > I just checked the standards and you're right, the appropriate behavior is let's not over-design this. sum(to_double(x)) is a workaround that avoids the overflow as you pointed out. if it ever becomes a performance issue then we'll optimize it at that point. there's a chance of precision loss when large long values are converted into double. that's what we're trying to avoid by using long for sum when inputs are integers. if user is ok with precision loss and wants to avoid overflow then user must manually specify it by using to_double() Line 64: protected ISerializerDeserializer serde = null; > Isn't each local/intermediate/global instance created differently when it's init()/finish() are called once per group on each partition if the aggregate is computed by the group-by operator. That's why it's better to create them in the constructor (once per partition) than in finish() (once per group) -- To view, visit https://asterix-gerrit.ics.uci.edu/3012 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I987417770b3bfbda6af29a27acc8c96dc8a99eb8 Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: James Fang <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-HasComments: Yes
