Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8833 )
Change subject: IMPALA-6300: Fix decimal modulo overflow ...................................................................... Patch Set 2: (2 comments) Other than deprecating GetScaleQuotient, this looks good. http://gerrit.cloudera.org:8080/#/c/8833/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8833/2/be/src/runtime/decimal-value.inline.h@182 PS2, Line 182: 73, 76, 79, 83, 86, 89, 93, 96, 99, 102, 106, 109, 112, 116, 119, 122, 126, 129 }; Why up to 10^40? Can we really scale that high? I would have thought the largest would be floor(log(10**38, 2)) http://gerrit.cloudera.org:8080/#/c/8833/2/be/src/runtime/decimal-value.inline.h@613 PS2, Line 613: *y_scaled = detail::SafeMultiply<RESULT_T>(y.value(), scale_factor, false); I checked and this is was the only usage of GetScaleQuotient outside of tests, and that appears to be a benchmark for this function (which is now changed). Maybe we should get rid of that function? I find it really confusing to think about, especially since the division by powers of MAX_DECIMAL by powers of 10 is not exact. -- To view, visit http://gerrit.cloudera.org:8080/8833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686 Gerrit-Change-Number: 8833 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Fri, 15 Dec 2017 22:24:45 +0000 Gerrit-HasComments: Yes
