Tim Armstrong 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 GetConsta GetIrConstant works for all result types but GetConstant<int> only works for ints. We need the switch statement if we're going to add more argument types. The code is in a slightly strange state because it's generalised to work for multiple types but only implemented for one type, but it seemed better to add the switch here so it was consistent with the rest of the generalised logic. 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 tem Done Line 454: > indentation glitch 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: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
