Dan Hecht has posted comments on this change. Change subject: IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions ......................................................................
Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: Line 103: DCHECK(false) << "NYI:" << type.DebugString(); why do we not need these still with the removal of the bail outs for CHAR? Line 462: void CodegenAnyVal::SetFromRawPtr(Value* raw_ptr) { this looks unused. remove? http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: Line 53: /// TYPE_TIMESTAMP/TimestampVal: { i64, i64 } decimal? http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 1071: // eliminate a branch per value. what code is this paragraph referring to? should it precede the Init() call above? Line 1079: && evaluator->intermediate_type().type != TYPE_TIMESTAMP) { IsTimestampType() for consistency now that most types have that? PS12, Line 1454: : maybe explain that we hand-generate a code sequence in this case. PS12, Line 1670: .type != TYPE_TIMESTAMP IsTimestampType()? PS12, Line 1742: We must use the unlowered type would be good to briefly explain why rather than just the what. Line 1811: "intermediate tuple desc"); when would this happen? http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: PS12, Line 630: ; remove http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: PS12, Line 143: the "a boolean value represented ..." ? like above comment. -- To view, visit http://gerrit.cloudera.org:8080/4655 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9dc21d1d676505d3617e1e4f37557397c4fb260 Gerrit-PatchSet: 12 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
