[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 8: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion

This changes the behaviour of applying an arithmetic operator to
constant DECIMAL and non-DECIMAL arguments. In DECIMAL_V1, this
caused an implicit conversion to floating point, which caused
users a lot of confusion in some cases. In DECIMAL_V2 the typing
rules are simplified: constant decimals are treated the same as any
other decimals.

Testing:
Added some expression tests for different arithmetic operators
and binary predicates (the two Expr subclasses that call
convertNumericLiteralsFromDecimal()).

Extended analyzer tests to test DECIMAL_V2 behaviour. Added many
additional test for various combinations of literals and non-literals to
get better coverage of existing and new behaviour.

Ran core tests.

Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Reviewed-on: http://gerrit.cloudera.org:8080/7916
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 256 insertions(+), 56 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 8: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1223/

-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 7: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7916/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1476: checkDecimalReturnType("select 
12345678901234567890123456789012345678",
> Feels more appropriate to put these into TestNumericLiteralMinMaxValues() a
Done.

I don't think it's necessary to use checkDecimalReturnType() since there's no 
behavioural difference. I guess I could extend testNumericLiteral() to try with 
both values to ensure that the literal type doesn't change but I'm not sure 
that it meaningfully improves coverage.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-14 Thread Tim Armstrong (Code Review) 
Hello Greg Rahn,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7916

to look at the new patch set (#7).

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..

IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion

This changes the behaviour of applying an arithmetic operator to
constant DECIMAL and non-DECIMAL arguments. In DECIMAL_V1, this
caused an implicit conversion to floating point, which caused
users a lot of confusion in some cases. In DECIMAL_V2 the typing
rules are simplified: constant decimals are treated the same as any
other decimals.

Testing:
Added some expression tests for different arithmetic operators
and binary predicates (the two Expr subclasses that call
convertNumericLiteralsFromDecimal()).

Extended analyzer tests to test DECIMAL_V2 behaviour. Added many
additional test for various combinations of literals and non-literals to
get better coverage of existing and new behaviour.

Ran core tests.

Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 256 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7916/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-13 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1331: checkDecimalReturnType("select 1 + 1.1",
> 1 -> interpreted as tinyint -> decimal(3, 0)
Ahh right, thanks for the refresher.


Line 1386: Type.DOUBLE, ScalarType.createDecimalType(2, 1));
> So I guess the current behaviour seems OK to me but I haven't thought deepl
Agree.


http://gerrit.cloudera.org:8080/#/c/7916/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1476: checkDecimalReturnType("select 
12345678901234567890123456789012345678",
Feels more appropriate to put these into TestNumericLiteralMinMaxValues() at 
the top of this file. There are already some existing tests that we should 
dedup with your new tests. Perhaps some of the existing tests there should also 
use checkDecimalReturnType()


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-13 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1386: Type.DOUBLE, ScalarType.createDecimalType(2, 1));
> That's the current behaviour and it does treat literals consistently with o
So I guess the current behaviour seems OK to me but I haven't thought deeply. 
Maybe it's worth revisiting but that's a separate discussion from handling of 
literals.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-13 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 513:* Converts numeric literal in the expr tree rooted at this expr to 
return floating
> literals
Done


Line 516:* Decimal has a higher processing cost than floating point and we 
should not pay
> Please add subsections "DECIMAL_V1" and "DECIMAL_V2" to explain the rationa
Done. I didn't expend many characters on the DECIMAL_V2 section but I think 
it's sufficient to explain the existence of the two code paths (since the 
comment will go away when DECIMAL_V1 goes away).


http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1306:* Test expressions that resolve to different types depend on the 
DECIMAL_V2 setting.
> Test expressions that return different decimal types depending on the DECIM
Reworded slightly since some don't return decimal types, just involve decimal 
somehow.


Line 1319:* Test expressions that resolve to the same type with either 
DECIMAL_V2 value.
> resolve to -> return
Done


Line 1331: Type.DOUBLE, ScalarType.createDecimalType(5, 1));
> Why is the result not DECIMAL(2,1)?
1 -> interpreted as tinyint -> decimal(3, 0)
1.1 -> interpreted as decimal(2,1)

Then adding decimal(3, 0) and decimal(2, 1) generally results in decimal(5, 1).

This makes sense to me. It's not perfect but it's consistent.

To get the tighter bound on the decimal type you'd need to interpret literals 
without decimal points as decimal literals (or do something even more 
complicated in the type system).


Line 1342: // DECIMAL_V2: floating point + decimal expr = decimal
> This does not seem to explain what happens for "floating point + numeric li
I don't think the original comment conveyed my intent well. 
The comment is intended to describe the general rules that apply for DECIMAL_V1 
and DECIMAL_V2. DECIMAL_V1's rules treat all numeric literals the same way in 
this context, whereas DECIMAL_V2's rules treat all decimal exprs the same way.

The R.H.S. expressions in these cases are decimal literals, so they are in the 
intersection of decimal exprs and numeric literals.


Line 1386: // Multiplying a floating point type with any other type always 
results in a double.
> Why? Seems inconsistent.
That's the current behaviour and it does treat literals consistently with other 
exprs. I think the idea is that multiplication is more likely to overflow 
fixed-point values. The code in getArithmeticResultType() is:

  // For multiplications involving at least one floating point type we cast 
decimal to
  // double in order to prevent decimals from overflowing.
  if (op == ArithmeticExpr.Operator.MULTIPLY &&
  (t1.isFloatingPointType() || t2.isFloatingPointType())) {
return Type.DOUBLE;
  }


Line 1409: checkDecimalReturnType("select round(1.2345, 2) * pow(10, 10)", 
Type.DOUBLE);
> add same test with "+"
Done. Also updated the comment because it was a little unclear that it was 
testing a very specific pattern instead of general expressions with decimal + 
double.


Line 1426: // Test behavior of compound expressions with a single column 
and many literals.
> column -> slot ref
Done


Line 1465: checkDecimalReturnType("select 1.123e-2", 
ScalarType.createDecimalType(5, 5));
> What about literals that don't fit into a decimal?
Added some more tests along those lines.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-13 Thread Tim Armstrong (Code Review) 
Hello Greg Rahn,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7916

to look at the new patch set (#6).

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..

IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion

This changes the behaviour of applying an arithmetic operator to
constant DECIMAL and non-DECIMAL arguments. In DECIMAL_V1, this
caused an implicit conversion to floating point, which caused
users a lot of confusion in some cases. In DECIMAL_V2 the typing
rules are simplified: constant decimals are treated the same as any
other decimals.

Testing:
Added some expression tests for different arithmetic operators
and binary predicates (the two Expr subclasses that call
convertNumericLiteralsFromDecimal()).

Extended analyzer tests to test DECIMAL_V2 behaviour. Added many
additional test for various combinations of literals and non-literals to
get better coverage of existing and new behaviour.

Ran core tests.

Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 258 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7916/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-12 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1331: Type.DOUBLE, ScalarType.createDecimalType(5, 1));
> Why is the result not DECIMAL(2,1)?
I mean DECIMAL(3,1) of course.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-12 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 5:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 513:* Converts numeric literal in the expr tree rooted at this expr to 
return floating
literals


Line 516:* Decimal has a higher processing cost than floating point and we 
should not pay
Please add subsections "DECIMAL_V1" and "DECIMAL_V2" to explain the rationale 
for their casting behavior. In particular, does decimal still have a higher 
processing cost?

This way it becomes clear that the old behavior was intended to maximize 
performance and the new behavior is intended to provide accurate/consistent 
behavior (but does it take a perf hit for that?)


Line 529:   protected void convertNumericLiteralsFromDecimal(Analyzer analyzer)
I can't wait for this function to be deleted.


http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1306:* Test expressions that resolve to different types depend on the 
DECIMAL_V2 setting.
Test expressions that return different decimal types depending on the 
DECIMAL_V2 setting.


Line 1319:* Test expressions that resolve to the same type with either 
DECIMAL_V2 value.
resolve to -> return

(in the FE 'resolve' has a specific meaning for SlotRefs and TableRefs which we 
should not confuse here)


Line 1331: Type.DOUBLE, ScalarType.createDecimalType(5, 1));
Why is the result not DECIMAL(2,1)?
Decimal type seems weird, but not the focus of this patch.


Line 1342: // DECIMAL_V2: floating point + decimal expr = decimal
This does not seem to explain what happens for "floating point + numeric 
literal" like in the DECIMAL_V1 case.

What happens in cases where the numeric literal is a float because it does not 
fit into our max decimal? Needs test.

Might be clearer if you list:
DECIMAL_V2: float + decimal literal = decimal
DECIMAL_V2: float + float literal = float


Line 1386: // Multiplying a floating point type with any other type always 
results in a double.
Why? Seems inconsistent.


Line 1409: checkDecimalReturnType("select round(1.2345, 2) * pow(10, 10)", 
Type.DOUBLE);
add same test with "+"


Line 1426: // Test behavior of compound expressions with a single column 
and many literals.
column -> slot ref

(and elsewhere)


Line 1465: checkDecimalReturnType("select 1.123e-2", 
ScalarType.createDecimalType(5, 5));
What about literals that don't fit into a decimal?


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-12 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..


Patch Set 5:

Updated with some additional tests to get more exhaustive coverage of the 
different possible permutations.

-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion

2017-09-12 Thread Tim Armstrong (Code Review) 
Hello Greg Rahn,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7916

to look at the new patch set (#5).

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double 
conversion
..

IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion

This changes the behaviour of applying an arithmetic operator to
constant DECIMAL and non-DECIMAL arguments. In DECIMAL_V1, this
caused an implicit conversion to floating point, which caused
users a lot of confusion in some cases. In DECIMAL_V2 the typing
rules are simplified: constant decimals are treated the same as any
other decimals.

Testing:
Added some expression tests for different arithmetic operators
and binary predicates (the two Expr subclasses that call
convertNumericLiteralsFromDecimal()).

Extended analyzer tests to test DECIMAL_V2 behaviour. Added many
additional test for various combinations of literals and non-literals to
get better coverage of existing and new behaviour.

Ran core tests.

Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 200 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7916/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn