Dan Hecht has posted comments on this change. Change subject: IMPALA-2020: Add rounding for decimal casts ......................................................................
Patch Set 5: (3 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)); > Not really - I thought the saner behavior for ToInt was rounding and wrote I don't feel too strongly about it, but think the symmetry in the interfaces is nice. If you leave the non-rounding case here, then line 303 should be formatted this way per our style: } else { return ... } We only omit line breaks and braces if the entire if-stmt fits on one line (and maybe doesn't have an else clause?). 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); > In what sense? Will Impala 2.9 and Impala 2.8 give different answers when decmial_v2=false? 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 > Done. Means I can't give a default value to round unless I also give a def Regarding default arguments, let's avoid adding them unless they really make things simpler. Often, they just lead to coding errors since new users of the interface don't bother to think about the right values to pass. Regarding default value for overflow, I don't think that makes sense since as you concluded, all callers should care about overflow. If they don't, that probably indicates a bug. -- 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
