Michael Ho has posted comments on this change. Change subject: IMPALA-4397,IMPALA-3259: reduce codegen time and memory ......................................................................
Patch Set 7: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/4956/7/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS7, Line 602: codegen It still doesn't track the memory consumed by the LLVM module (e.g. Instruction, BasicBlock objects), right ? http://gerrit.cloudera.org:8080/#/c/4956/7/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: PS7, Line 809: ctxs.size() > Expr::CODEGEN_INLINE_EXPRS_THRESHOLD nit: May be worth refactoring it into a singe variable with meaningful name for easier understanding. http://gerrit.cloudera.org:8080/#/c/4956/7/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS7, Line 1706: if (aggregate_evaluators_.size() > Expr::CODEGEN_INLINE_EXPRS_THRESHOLD) { As discussed in person, it may be worth double checking whether min / max / sum on wide table doesn't regress as UpdateSlot() is rather small for these aggregate evaluators. It probably doesn't but it'd be good to avoid surprises. For instance, select min(col1),max(col1),sum(col1),,....,min(col26),max(col26),sum(col26) from wide_table makes a difference without inlining for UpdateSlot() into UpdateTuple(). -- To view, visit http://gerrit.cloudera.org:8080/4956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id10015b49da182cb181a653ac8464b4a18b71091 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
