You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/06/28 16:44:17 UTC
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7316
Change subject: IMPALA-5591: set should handle negative values
......................................................................
IMPALA-5591: set should handle negative values
The parser didn't account for the possibility of negative
numeric literals.
Testing:
Added a test that sets a negative value. Query tests send the whole
"set" statement to the backend for execution so exercise the parser.
Ran core tests.
Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
---
M fe/src/main/cup/sql-parser.cup
M testdata/workloads/functional-query/queries/QueryTest/set.test
2 files changed, 15 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7316/1
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/806/
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 3: Code-Review+2
carry +2
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 2: Code-Review+2
Makes sense, thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
IMPALA-5591: set should handle negative values
The parser didn't account for the possibility of negative
numeric literals.
Testing:
Added a test that sets a negative value. Query tests send the whole
"set" statement to the backend for execution so exercise the parser.
Ran core tests.
Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Reviewed-on: http://gerrit.cloudera.org:8080/7316
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/cup/sql-parser.cup
M testdata/workloads/functional-query/queries/QueryTest/set.test
2 files changed, 15 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7316/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS1, Line 2291:
> I need to remove this tab.
Agree, that looks like a more involved change to resolve the ambiguity. Thanks for clarifying, I'm ok with this approach.
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7316/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS1, Line 2291:
I need to remove this tab.
PS1, Line 2291: KW_SET ident_or_default:key EQUAL SUBTRACT numeric_literal:l
> How about making changes in sql-scanner to accommodate -ve integer literals
I think that would make the grammar ambiguous (at least without other changes) since "-" is also an operator, not just part of a literal. I.e. if "SUBTRACT expr" remains a rule, then "-1" could be parsed as either "literal" or "SUBTRACT literal".
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 2:
Do we have query options where negative values are meaningful?
I'm a little confused by the JIRA. How do we end up with "-1" when we "set scratch_limit=0;"?
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7316/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS1, Line 2291: KW_SET ident_or_default:key EQUAL SUBTRACT numeric_literal:l
How about making changes in sql-scanner to accommodate -ve integer literals? Reason being we can have other places that could potentially require to use -ve integers/decimals and we need to implement this branching every where duplicating parser code. Thoughts?
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7316
to look at the new patch set (#2).
Change subject: IMPALA-5591: set should handle negative values
......................................................................
IMPALA-5591: set should handle negative values
The parser didn't account for the possibility of negative
numeric literals.
Testing:
Added a test that sets a negative value. Query tests send the whole
"set" statement to the backend for execution so exercise the parser.
Ran core tests.
Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
---
M fe/src/main/cup/sql-parser.cup
M testdata/workloads/functional-query/queries/QueryTest/set.test
2 files changed, 15 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/7316/2
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 2:
-1 means "unlimited" for scratch_limit since 0 is a reasonable value. It's a bit unfortunate that 0 is used to mean "unlimited" for some other query options.
The default value of scratch_limit is -1, so when it's overridden in a .test file it gets automatically set back to -1 at the end of the test.
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5591: set should handle negative values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5591: set should handle negative values
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7316
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c415dbed6ba1122919be75f5811444d88ee03b4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No