Tim Armstrong has posted comments on this change.

Change subject: Inject precision and scale constants for all decimal builtins
......................................................................


Patch Set 6:

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


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
Done


Line 297:   int InlineConstants(LlvmCodeGen* codegen, llvm::Function* fn);
> Maybe keep this signature protected, since it's meant to be used by Expr su
Done


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?
Ah, forgot to fix this up. Done.


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

Reply via email to