Hussain Towaileb 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; > we need to sum into a long value if all inputs are integers. This problem e Currently I'm only doing the sum in a local double instance but the output of each function (finish() call) is actually outputting int64 or double based on the incoming data types, please see the else statement at line 199 in the finishFinal() method. As for the overflow exception, although your approach is probably the correct way, I intentionally converted the result to double instead of int64 if the Long.MAX_VALUE is exceeded, the reason behind that is: (and I would like your input on this please) Say the user has a close type with a field (myValue) of type int64, after a period of time, the total sum of the field is exceeding the Long.MAX_VALUE and hence will always result in an overflow exception, what can the user do in this case to be able to perform the sum successfully? would sum(to_double(myValue)) be the option to go with to solve this, and won't this impact the performance? And is this convenient or too technical for a normal user? I would appreciate your advise/feedback on this. I'll go ahead and make the change as suggested, but I'll postpone pushing it till receiving the feedback, thanks! Line 64: protected ISerializerDeserializer serde = null; > we only have two kinds of serdes now, so let's just create both of the here Isn't each local/intermediate/global instance created differently when it's needed, and hence, for each instance, the finish() function is called only once per instance? (This is just for my information) Although, I totally agree with you, since we only have 2, creating them both ahead of time seems like the right/safer approach, I'll go ahead and do that! I thought I was saving few bytes by doing the above. :) -- 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
