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-4821: Update AVG() for DECIMAL V2
Date Thu, 16 Feb 2017 18:28:12 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4821: Update AVG() for DECIMAL_V2
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6038/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

Line 450:     DCHECK_EQ(sum_scale, output_scale);
I don't think we need this DCHECK since it was just an artificial restriction put on the code.


http://gerrit.cloudera.org:8080/#/c/6038/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 408:     size_t from_offset = expr.find("from");
please add a short comment explaining this.
also, expr is no longer an expression..


http://gerrit.cloudera.org:8080/#/c/6038/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 335:         analyzer.getQueryOptions().isDecimal_v2()) {
let's add a comment:
// AVG() result always gets at least MIN_ADJUSTED_SCALE decimal places since it performs an
implicit divide.


Line 338:       return ScalarType.createAdjustedDecimalType(resultPrecision, resultScale);
I guess this works out as the same type as falling through and taking the createClippedDecimalType()
path, right? if so, i'm fine with either (falling through might be less "tricky", and we are
clipping the precision in this case anyway).


-- 
To view, visit http://gerrit.cloudera.org:8080/6038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I28f5ef0370938440eb5b1c6d29b2f24e6f88499f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message