impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4370: Divide and modulo result types for DECIMAL version V2
Date Mon, 13 Feb 2017 21:51:14 GMT
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 <dhecht@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message