Tim Armstrong has posted comments on this change. Change subject: IMPALA-3066: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3220/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 76: DEFINE_bool(disable_lazy_materialization, false, > In case anything breaks, we can revert to eager materialization mode. Maybe we should chat a bit offline. If there are some gaps in test coverage that mean we're not confident in the lazy materialization, it would probably help to address them. My concern is a) adding more options/complexity b) we won't be testing the "safe" option, so bugs could creep in there. Line 97: static StringRef llvm_module_ir; > These are local to this file so not too bad IMHO. They are initialized once It's still some additional effort to reason about when they are initialised, etc, and I don't see any benefit. Line 797: nullptr > LLVM uses nullptr. Don't think this is actually the same as NULL. There are some obscure type differences but I think NULL should work ok (it's what we use everywhere). http://gerrit.cloudera.org:8080/#/c/3220/1/be/src/runtime/buffered-tuple-stream.inline.h File be/src/runtime/buffered-tuple-stream.inline.h: Line 58 > Not really. Just happened to see this opportunity when debugging some stuff I'm ok with this, maybe just mention in commit message? -- To view, visit http://gerrit.cloudera.org:8080/3220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ed7862fc5e86005ecea83fa2ceb489e737d66b2 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
