Tim Armstrong 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? The char bailout was still there, but more implicit. Line 462: void CodegenAnyVal::SetFromRawPtr(Value* raw_ptr) { > this looks unused. remove? Done 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? Done 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 It probably makes sense to move it up, since it's implemented by both Init() and the below code. Line 1079: && evaluator->intermediate_type().type != TYPE_TIMESTAMP) { > IsTimestampType() for consistency now that most types have that? Done PS12, Line 1454: : > maybe explain that we hand-generate a code sequence in this case. Done PS12, Line 1670: .type != TYPE_TIMESTAMP > IsTimestampType()? Done PS12, Line 1742: We must use the unlowered type > would be good to briefly explain why rather than just the what. Done Line 1811: "intermediate tuple desc"); > when would this happen? If there's a char field in the tuple. I think this is a step back in terms of error messages, so I added back an explicit check for CHAR fields in the intermediate tuple. I actually noticed there are a couple of places in the codebase that don't check for a null result from this function, so I fixed those. 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 Done 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. Done -- 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
