Tim Armstrong has posted comments on this change.

Change subject: IMPALA-775,IMPALA-3374: Upgrade LLVM to 3.8.0
......................................................................


Patch Set 11:

(4 comments)

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 
We should get an error propagated up to the user even if it switches to 
codegen. For IR udfs, the query fails. We could consider modifying the test 
framework to assert that there are no errors or warnings by default.


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?
Done


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?
There was actually an error template so used that.


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.
Done


-- 
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

Reply via email to