You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/02/14 00:04:40 UTC

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py 

Michael Ho has uploaded a new change for review.

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

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................

IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py

This change enables decimal_v2 for casting to decimal except
for cases which involve rounding/truncation. In which case,
we only enable decimal_v2 for casting from numbers to decimal.
Due to prior misunderstanding, some of the changes made in
commit 2088cf2 weren't necessary. By default, Impala interprets
floating numbers as decimal unless they exceed the maximum
decimal type supported. This change also removess a minor quirk:
previously, the test will revert to using string to decimal
casting for numbers larger than 2^63-1. This is unnecessary as
Impala is able to treat numbers larger than that bound as
decimal type.

Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
---
M tests/query_test/test_decimal_casting.py
1 file changed, 23 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5989/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5989/1/tests/query_test/test_decimal_casting.py
File tests/query_test/test_decimal_casting.py:

PS1, Line 34: casts from floats
> is this true? i don't see where it casts from float to decimal. maybe this 
Done


PS1, Line 48: precisin
> precision
Done


Line 83:     # TODO: Fix casting from double to decimal for precision > 38.
> what does this mean? today it truncates on double to decimal cast.
Due to IMPALA-4936, the cast from double to decimal doesn't always return null when it overflows. So, use casting from string for now to verify the behavior for now.


PS1, Line 161: is_decimal_v2
> how about:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Zach Amsden,

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

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

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

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................

IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py

This change enables decimal_v2 for casting to decimal except
for cases which involve rounding/truncation. In which case,
we only enable decimal_v2 for casting from numbers to decimal.
Due to prior misunderstanding, some of the changes made in
commit 2088cf2 weren't necessary. By default, Impala interprets
floating numbers as decimal unless they exceed the maximum
decimal type supported. This change also removess a minor quirk:
previously, the test will revert to using string to decimal
casting for numbers larger than 2^63-1. This is unnecessary as
Impala is able to treat numbers larger than that bound as
decimal type.

Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
---
M tests/query_test/test_decimal_casting.py
1 file changed, 27 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5989/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5989/1//COMMIT_MSG
Commit Message:

PS1, Line 9: This change enables decimal_v2 for casting to decimal except
           : for cases which involve rounding/truncation
> What about for DECIMAL to DECIMAL casting that does involve rounding? Is th
Yes. Please see test_underflow().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/273/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py

This change enables decimal_v2 for casting to decimal except
for cases which involve rounding/truncation. In which case,
we only enable decimal_v2 for casting from numbers to decimal.
Due to prior misunderstanding, some of the changes made in
commit 2088cf2 weren't necessary. By default, Impala interprets
floating numbers as decimal unless they exceed the maximum
decimal type supported. This change also removess a minor quirk:
previously, the test will revert to using string to decimal
casting for numbers larger than 2^63-1. This is unnecessary as
Impala is able to treat numbers larger than that bound as
decimal type.

Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Reviewed-on: http://gerrit.cloudera.org:8080/5989
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M tests/query_test/test_decimal_casting.py
1 file changed, 27 insertions(+), 36 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5989/1//COMMIT_MSG
Commit Message:

PS1, Line 9: This change enables decimal_v2 for casting to decimal except
           : for cases which involve rounding/truncation
What about for DECIMAL to DECIMAL casting that does involve rounding? Is that enabled now?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5989/1/tests/query_test/test_decimal_casting.py
File tests/query_test/test_decimal_casting.py:

PS1, Line 34: casts from floats
is this true? i don't see where it casts from float to decimal. maybe this comment predates a change to make literals treated as decimal.
also, this test does test cast decimal to decimal, right? can you update this comment to explain what it covers?
let's add a TODO for testing float/double to decimal.


PS1, Line 48: precisin
precision


Line 79:     assert expected == actual, "Cast: {0}, Expected: {1}, Actual: {2}".format(cast,\
> +1 internets
what?


Line 83:     # TODO: Fix casting from double to decimal for precision > 38.
what does this mean? today it truncates on double to decimal cast.


PS1, Line 161: is_decimal_v2
how about:

is_decimal_v2 and cast_from != "string"

and then delete line 151-152 so we still exercise string in v2? Then when we do cast from string with rounding we just delete the second clause here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4809: Enable support for DECIMAL V2 in decimal casting.py

Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.

Change subject: IMPALA-4809: Enable support for DECIMAL_V2 in decimal_casting.py
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5989/1/tests/query_test/test_decimal_casting.py
File tests/query_test/test_decimal_casting.py:

Line 79:     assert expected == actual, "Cast: {0}, Expected: {1}, Actual: {2}".format(cast,\
+1 internets


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf2c8c9d360ad92cbdc5ce902ee742ec4408a8a3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes