Tim Armstrong has posted comments on this change. Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen ......................................................................
Patch Set 1: (8 comments) The general approach looks good - mainly the comments are trying to make it easier to understand how the different functions and data structures fit together. http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 119: Value Function and GlobalVariable are both subclasses of GlobalObject so it would clearer if this was vector<GlobalObject*>. Maybe it could be called 'global_users' or something like that? PS1, Line 124: getParent()->getParent() getFunction()? PS1, Line 149: fn_name Is this right? It doesn't reference any variables that are updated within the loop. Line 382: for (const string& gv: gv_with_fns_) { This is the only place this is used, so we could instead just directly store the set of functions to be materialised. I think it would be easier to understand the purpose of a set called 'always_materialize_fns_' or something along those lines. http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS1, Line 511: FindUses FindGlobalUses? Line 516: static void FillCalleeMap(llvm::Function* fn, CalleeMap& callee_map, The name and comment should communicate that this excludes functions defined in the impalad daemon. http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: Line 57 It seems like we want to be able to inline the constants into the timestamp functions below. Maybe move the constant values into the header so they're visible? I.e. have const int64_t MAX_YEAR = 10000; in the header and const int64_t TimestampFunctions::MAX_YEAR; in timestamp-functions.cc http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: Line 115: Status InitForBeTests(); I think we should avoid replicating the InitForFeTests() pattern - it's kind of confusing. It's really not obvious that InitForBeTests() should force codegen in that place. It would probably make more sense to have a static flag in FeSupport that specifically forces codegen - then it's easier to understand why codegen is enabled for expr-test. -- To view, visit http://gerrit.cloudera.org:8080/5732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
