Dan Hecht has posted comments on this change. Change subject: IMPALA-775,IMPALA-3374: Upgrade LLVM to 3.8.0 ......................................................................
Patch Set 11: Code-Review+2 (4 comments) Looks good to me. Please let Michael finish his review as well. http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 545: return false; now that the DCHECK is gone, will we still notice during testing when this thing returning false? Maybe we just rely on perf testing for that, since as you pointed out, a malformed UDF IR file could trigger this path (and so we can't dcheck). http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 292: VerifyFunction can this be private? http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 391: return Status("Function codegen failed verification: see error log for details."); maybe put the udf name in the error? http://gerrit.cloudera.org:8080/#/c/2486/11/bin/impala-config.sh File bin/impala-config.sh: Line 255: # Debug builds should use the release+asserts build to get additional coverage. briefly explain why not using llvm debug build. -- To view, visit http://gerrit.cloudera.org:8080/2486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d7afd05ad3b472a0bfe035bfc3daada5597b2d Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
