Skye Wanderman-Milne has posted comments on this change. Change subject: Inject precision and scale constants for all decimal builtins ......................................................................
Patch Set 6: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/2535/6/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 637: // Increment iter now so we don't mess it up modifying the instrunction below Can you fix 'instrunction' while you're here? http://gerrit.cloudera.org:8080/#/c/2535/6/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 276: // InlineConstants() must be run on the function to replace recognized constants. The InlineConstants() doesn't have to be run, the interpreted path does not run InlineConstants(). This probably isn't what you meant but I would rephrase it so it doesn't sound like running InlineConstants() is required. Line 297: int InlineConstants(LlvmCodeGen* codegen, llvm::Function* fn); Maybe keep this signature protected, since it's meant to be used by Expr subclasses directly. http://gerrit.cloudera.org:8080/#/c/2535/6/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 486: InlineConstants(return_type, arg_types, codegen, *udf); Use the original InlineConstants signature? -- To view, visit http://gerrit.cloudera.org:8080/2535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b7d1485f357ba88517ca15c5c7428c0ffe25dfd Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
