Dan Hecht has posted comments on this change. Change subject: Inject precision and scale constants for all decimal builtins ......................................................................
Patch Set 8: (1 comment) 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: > GetIrConstant works for all result types but GetConstant<int> only works fo but what's an example of a non-int constant that would be derived from arg_types/return_type? really, the template generalization seems overkill because the caller always has to know the type anyway and each constant would have a specific type, so code could just as well call e.g. GetConstantInt(). In other words, when would a generic implementation of GetConstant() be used? I'm not saying we can't use templates here if there's a good reason, but it'd be nice to have an example in mind that shows why templates help. in any case, if we do stick with this logic, let's at least explain in the GetIrConstant header comment that it's a wrapper to create an IR constant of the correct type. And conceptually it seems cleaner to derive the type from the symbol rather than enum value, but I guess that'd be more complicated code so probably not worth it. Alternatively, we could encode the type in ExprConstant value so that we can switch off the type directly. But again, maybe not worth it. -- 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
