Zach Amsden 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' Not really - I thought the saner behavior for ToInt was rounding and wrote this cast first, which didn't require a parameter, whereas FromDouble does as it is used in places where no context is available. I can add this if you think it is useful. 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? In what sense? 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. Done. Means I can't give a default value to round unless I also give a default value to overflow, but lets do that anyway. Is it okay if I change the ABI here to be >From Double (const ColumnType& t, double d, bool round = true, bool& overflow >= of_ignored) and then have a static bool DecimalValue::of_ignored which will get inserted when the caller doesn't bother to check overflow. (Actually, Q: can reference arguments even have default values?) The reason being to prevent a nullptr deref crash. Worth having a debate about references vs. pointers here. Passing a pointer allows us to omit the overflow check from ever happening (ptr == nullptr should be known at compile time) but we currently don't do that or null checks in any of the places where we pass back this style of overflow. I think we probably will want the overflow check in all runtime cases so maybe this doesn't matter. Line 232: inline typename RESULT_T::underlying_type_t ToInt(int scale, bool& overflow) const; > add a function comment. Done PS5, Line 232: bool& overflow > use 'bool* overflow'. we generally use pointers for output params to make i You have answered my last question. Please ignore 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 up Yeah that is bogus. What we need is the maximum integer value that represents a base 10 number of length precision, so the -scale should go. -- 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 <zams...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
