[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5722: Fix string to decimal cast
..


IMPALA-5722: Fix string to decimal cast

When converting a string to a decimal, we didn't handle the case where
the exponent is a large negative number. The function for computing the
divisor returns -1 when the exponent is too large. The problem is fixed
by making sure that the divisor is positive.

Testing:
-Added decimal tests.

Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Reviewed-on: http://gerrit.cloudera.org:8080/7517
Reviewed-by: Tim Armstrong 
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/decimal-test.cc
M be/src/util/decimal-util.h
M be/src/util/string-parser.h
3 files changed, 31 insertions(+), 6 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Jim Apple: Looks good to me, approved
  Tim Armstrong: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2).

Change subject: IMPALA-5722: Fix string to decimal cast
..

IMPALA-5722: Fix string to decimal cast

When converting a string to a decimal, we didn't handle the case where
the exponent is a large negative number. The function for computing the
divisor returns -1 when the exponent is too large. The problem is fixed
by making sure that the divisor is positive.

Testing:
-Added decimal tests.

Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
---
M be/src/runtime/decimal-test.cc
M be/src/util/decimal-util.h
M be/src/util/string-parser.h
3 files changed, 31 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 2: Code-Review+1

Looks good to me. Will give Jim a chance to weigh in if he has further comments.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/runtime/decimal-test.cc
File be/src/runtime/decimal-test.cc:

Line 305:   // When the absolute value of the exponent becomes too large, it is 
considered
> This seems a bit weird - it seems like it should be underflow. It seems lik
Changed the be code so that a large negative number would lead to an underflow 
(instead of overflow).


http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

Line 63:   DCHECK_GE(result * 10, result);
> The generic integer overflow support wasn't added until gcc5 I believe. Agr
Added a TODO.


http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

PS1, Line 225: DCHECK
> DCHECK_EQ
DCHECK_EQ doesn't work with int128. Added a comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2).

Change subject: IMPALA-5722: Fix string to decimal cast
..

IMPALA-5722: Fix string to decimal cast

When converting a string to a decimal, we didn't handle the case where
the exponent is a large negative number. The function for computing the
divisor returns -1 when the exponent is too large. The problem is fixed
by making sure that the divisor is positive.

Testing:
-Added decimal tests.

Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
---
M be/src/runtime/decimal-test.cc
M be/src/util/decimal-util.h
M be/src/util/string-parser.h
3 files changed, 31 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

Line 63:   DCHECK_GE(result * 10, result);
> If T is signed, the compiler is permitted to turn this into a no-op: http:/
The generic integer overflow support wasn't added until gcc5 I believe. Agree 
that this isn't the ideal way to check for overflow though - maybe add a TODO 
indicating that the check is imperfect for this reason.


http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 221: T divisor = DecimalUtil::GetScaleMultiplier(shift);
> Look at decimal-util.h line 174, where -1 is returned. The function you wer
Thanks, that makes more sense.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 221: T divisor = DecimalUtil::GetScaleMultiplier(shift);
> I think it can be a huge negative number once it overflows.
Look at decimal-util.h line 174, where -1 is returned. The function you were 
looking at, Tim, is called only in the case of int256.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[anujphadke (Code Review)]

anujphadke has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 221: T divisor = DecimalUtil::GetScaleMultiplier(shift);
> I must be missing something, but I don't understand how divisor can ever be
I think it can be a huge negative number once it overflows.

Ex when T is an int -
  int result = 1;
  for(int i=0; i <= 11; i++) {
result *= 10;
  }

result is 
-727379968


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

Line 63:   DCHECK_GE(result * 10, result);
If T is signed, the compiler is permitted to turn this into a no-op: 
http://eel.is/c++draft/expr#4 , https://blog.regehr.org/archives/1520

You could use make_unsigned_t to do the multiplication in the unsigned domain 
and then convert back or you could use 
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html.

Using make_unsigned is tricky for __int128.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5722: Fix string to decimal cast
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/runtime/decimal-test.cc
File be/src/runtime/decimal-test.cc:

Line 305:   // When the absolute value of the exponent becomes too large, it is 
considered
This seems a bit weird - it seems like it should be underflow. It seems like 
this might lead to CastToDecimalVal() returning the wrong thing, since it has 
different behaviour on underflow and overflow.


http://gerrit.cloudera.org:8080/#/c/7517/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 221: T divisor = DecimalUtil::GetScaleMultiplier(shift);
I must be missing something, but I don't understand how divisor can ever be -1, 
since GetScaleMultiplier returns 1 multiplied by "scale" 10s.


PS1, Line 225: DCHECK
DCHECK_EQ


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5722: Fix string to decimal cast

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new change for review.

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

Change subject: IMPALA-5722: Fix string to decimal cast
..

IMPALA-5722: Fix string to decimal cast

When converting a string to a decimal, we didn't handle the case where
the exponent is a large negative number. The function for computing the
divisor returns -1 when the exponent is too large. The problem is fixed
by making sure that the divisor is positive.

Testing:
-Added decimal tests.

Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
---
M be/src/runtime/decimal-test.cc
M be/src/util/decimal-util.h
M be/src/util/string-parser.h
3 files changed, 23 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1f5eb5b64a6924af2e8eb7f9a50da67015757efe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky