impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2020: Add rounding for decimal casts
Date Tue, 14 Feb 2017 00:06:14 GMT
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 <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message