Dan Hecht has posted comments on this change. Change subject: IMPALA-4370: Divide and modulo result types for DECIMAL version V2 ......................................................................
Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/5952/8/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS8, Line 228: other > not your change but it may be clearer to call this divisor. All the other operators use the terminology 'this' and 'other', so would like to stay consistent, unless you feel strongly. http://gerrit.cloudera.org:8080/#/c/5952/8/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java: Line 100: ArithmeticExpr.Operator op, TQueryOptions queryOptions) throws AnalysisException { > instead of queryoptions, how about just passing a bool in here? narrower in Done. I thought it was slightly nicer to have the decimal code itself make the decision rather than make that decision in the generic arithmetic logic. But I don't feel too strongly. Line 152: * on whether DECIMAL version 1 or DECIMAL version 2 is enabled. > since we're going to toss out the v1 behavior when we sunset cdh5, leave a Sure, but it should be easy to find by tracing down DECIMAL_V2 uses. Filed IMPALA-4924 and added a TODO here. Line 233: * precision. But an algorithm of reducing scale to a minimum reduction of 6 is > "to a minimum of 6" Done http://gerrit.cloudera.org:8080/#/c/5952/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: Line 138: public static ScalarType createDecimalTypeClipPrecScale(int precision, int scale) { > how about simply createClippedDecimalType()? or truncated instead of clippe Didn't want to use "truncate" since we use that terminology in the backend to describe what happens when you lose fractional digits, whereas here we are losing whole number digits on the type. So went with createClippedDecimalType(). Added the preconditions check (this is why I made createDecimalTypeWildCard() but then forgot to do that. Also renamed that to createWildCardDecimalType() for consistency. http://gerrit.cloudera.org:8080/#/c/5952/8/tests/query_test/test_decimal_queries.py File tests/query_test/test_decimal_queries.py: PS8, Line 51: new_vector.get_value('exec_option')['decimal_v2'] = vector.get_value('decimal_v2') : new_vector.get_value('exec_option')['batch_size'] = vector.get_value('batch_size') > or simply do cls.ImpalaTestMatrix.add_dimension(create_exec_option_dimensio Ah, didn't know about that. Done. -- To view, visit http://gerrit.cloudera.org:8080/5952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I83e7f7787edfa4b4bddc25945090542a0e90881b Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
