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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
