Dan Hecht has posted comments on this change. Change subject: Inject precision and scale constants for all decimal builtins ......................................................................
Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/2535/8/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 614: case ARG_TYPE_SCALE: why have this switch statement? given that both GetIrConstant and GetConstant both take the same args, it seems reasonable that all constants would be computed via GetConstant(). Which cases might not? And then, I'm not sure that GetIrConstant() adds any more clarity. Seems it would be more straight forward to just move this one line into InlineConstants. http://gerrit.cloudera.org:8080/#/c/2535/8/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 282: newline (though I guess we are pretty inconsistent about formatting for templated func decls). Line 454: indentation glitch -- 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: 8 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
