You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2017/02/17 00:51:13 UTC
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Dan Hecht has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6044
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
IMPALA-4877: fix precedence of unary -/+
Currently, expressions such as "-2 / 3" parse as "-(2 / 3)". In
practice this usually doesn't cause differences for most common
expressions. However, it does interact with a heuristic that changes
decimal expression types to double when one operand is non-decimal
(see JIRA). For example, before this fix,
typeof(2.0/3.0) = DECIMAL
typeof(-2.0/3.0) = DOUBLE
With this fix, both expressions result in a DECIMAL.
Technically this is a compatibility breaking change but it seems
like no one would expect the current behavior so I think we should
fix it. Let me know if you disagree.
Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
---
M be/src/exprs/expr-test.cc
M fe/src/main/cup/sql-parser.cup
2 files changed, 15 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/6044/1
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 4:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/302/
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 4: Code-Review+2
Carry Marcel's +2.
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 5: Code-Review+2
rebase
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6044/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1382: { "2.0 / 3.0",
add variant -2.0/-3.0
Line 1385: { "-2.0 / 3.0",
add variants -(2.0/3.0) and 2.0/-3.0
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6044/2/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
Line 517: precedence left UMINUS, UPLUS;
actually, since they're identical, why not just have a single 'unarysign' or something like that?
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/313/
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 4: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/302/
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6044/2/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
Line 517: precedence left UMINUS, UPLUS;
> actually, since they're identical, why not just have a single 'unarysign' o
good point. done.
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6044/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1382: { "2.0 / 3.0",
> add variant -2.0/-3.0
Done
Line 1385: { "-2.0 / 3.0",
> add variants -(2.0/3.0) and 2.0/-3.0
Added 2.0/-3.0, but -(2.0/3.0) still produces a double. That's how "-2.0/3.0" was being parsed before this change, and results in a double due to IMPALA-3437.
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has uploaded a new patch set (#2).
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
IMPALA-4877: fix precedence of unary -/+
Currently, expressions such as "-2 / 3" parse as "-(2 / 3)". In
practice this usually doesn't cause differences for most common
expressions. However, it does interact with a heuristic that changes
decimal expression types to double when one operand is non-decimal
(see JIRA). For example, before this fix,
typeof(2.0/3.0) = DECIMAL
typeof(-2.0/3.0) = DOUBLE
because the latter expression parsed as "-1 * (2.0 / 3.0)".
With this fix, both expressions result in a DECIMAL.
Technically this is a compatibility breaking change but it seems
like no one would expect the current behavior so I think we should
fix it. Let me know if you disagree.
Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
---
M be/src/exprs/expr-test.cc
M fe/src/main/cup/sql-parser.cup
2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/6044/2
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6044
to look at the new patch set (#3).
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
IMPALA-4877: fix precedence of unary -/+
Currently, expressions such as "-2 / 3" parse as "-(2 / 3)". In
practice this usually doesn't cause differences for most common
expressions. However, it does interact with a heuristic that changes
decimal expression types to double when one operand is non-decimal
(see JIRA). For example, before this fix,
typeof(2.0/3.0) = DECIMAL
typeof(-2.0/3.0) = DOUBLE
because the latter expression parsed as "-1 * (2.0 / 3.0)".
With this fix, both expressions result in a DECIMAL.
Technically this is a compatibility breaking change but it seems
like no one would expect the current behavior so I think we should
fix it. Let me know if you disagree.
Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
---
M be/src/exprs/expr-test.cc
M fe/src/main/cup/sql-parser.cup
2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/6044/3
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4877: fix precedence of unary -/+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4877: fix precedence of unary -/+
......................................................................
IMPALA-4877: fix precedence of unary -/+
Currently, expressions such as "-2 / 3" parse as "-(2 / 3)". In
practice this usually doesn't cause differences for most common
expressions. However, it does interact with a heuristic that changes
decimal expression types to double when one operand is non-decimal
(see JIRA). For example, before this fix,
typeof(2.0/3.0) = DECIMAL
typeof(-2.0/3.0) = DOUBLE
because the latter expression parsed as "-1 * (2.0 / 3.0)".
With this fix, both expressions result in a DECIMAL.
Technically this is a compatibility breaking change but it seems
like no one would expect the current behavior so I think we should
fix it. Let me know if you disagree.
Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Reviewed-on: http://gerrit.cloudera.org:8080/6044
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/expr-test.cc
M fe/src/main/cup/sql-parser.cup
2 files changed, 21 insertions(+), 0 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>