skambha edited a comment on pull request #29026: URL: https://github.com/apache/spark/pull/29026#issuecomment-655252629
@cloud-fan, Thanks for looking into covering some more cases. If we change the UnsafeRow.setDecimal overflow logic, I agree the implementation of the sum needs to change. This is coming back to the discussion we had earlier here (https://github.com/skambha/spark/pull/1), the sum overflow logic is very much dependent on the underlying implementation of overflow logic in UnsafeRowWriter and UnsafeRow etc. I have a few comments related to the fix in UnsafeRow. 1) So now we have UnsafeRow.setDecimal silently returns a null for an overflowed decimal value in setDecimal, but getDecimal throws error. There is inconsistency here. Why is that ok? Also, they dont honor the ansi mode. 2) In this scenario, now we are more aggressive in the checking of the overflow. We have moved the overflow check further down to return null. Earlier I think the decision was to not do the checking per row, but now dont we end up doing that in some of the cases, right? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
