Dan Hecht has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/5951/5/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS5, Line 303: return to_type(dv.whole_part(scale)); any reason not to have ToInt<>() handle this case too by passing in 'round' (similar to FromDouble())? http://gerrit.cloudera.org:8080/#/c/5951/5/be/src/exprs/literal.cc File be/src/exprs/literal.cc: Line 185: value_.decimal4_val = Decimal4Value::FromDouble(type, v, &overflow, true); where is this used? will it break compatibility? http://gerrit.cloudera.org:8080/#/c/5951/5/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: PS5, Line 54: bool* overflow, : bool round here and below, let's keep input parameters first, then output params. PS5, Line 232: bool& overflow use 'bool* overflow'. we generally use pointers for output params to make it clearer at the callsite. Line 232: inline typename RESULT_T::underlying_type_t ToInt(int scale, bool& overflow) const; add a function comment. http://gerrit.cloudera.org:8080/#/c/5951/5/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 50: if (abs(d) >= max_value) { is this correct? 'd' is already scaled here. please be sure to test the upper bounds. -- To view, visit http://gerrit.cloudera.org:8080/5951 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
