You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Zoltan Haindrich <ki...@rxd.hu> on 2016/09/30 15:08:20 UTC
Review Request 52427: HIVE-14580 - Introduce || operator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/
-----------------------------------------------------------
Review request for hive, Ashutosh Chauhan and pengcheng xiong.
Repository: hive-git
Description
-------
adds support for || text concatenation
* reuses the concat udf for doing the real work
* same precedence as + / - ; I think this is expected
* small ast rewrite code in SemanticAnalyzer
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g b623187a788a23eceb70a71de62671ad778a24b7
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g e6b70a0a0e328a27a808032857b8ecb7ecffeb64
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/52427/diff/
Testing
-------
Thanks,
Zoltan Haindrich
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by pengcheng xiong <px...@hortonworks.com>.
> On Sept. 30, 2016, 5:30 p.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 434
> > <https://reviews.apache.org/r/52427/diff/1/?file=1516884#file1516884line434>
> >
> > Could u test if the precedence is correct w.r.t the other DBMS? If you put it here, it will be lower than */-, etc.
Sorry, it should be "it will be the same as +- and lower than */, etc
- pengcheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/#review151046
-----------------------------------------------------------
On Sept. 30, 2016, 3:08 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52427/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2016, 3:08 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and pengcheng xiong.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> adds support for || text concatenation
>
> * reuses the concat udf for doing the real work
> * same precedence as + / - ; I think this is expected
> * small ast rewrite code in SemanticAnalyzer
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g b623187a788a23eceb70a71de62671ad778a24b7
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g e6b70a0a0e328a27a808032857b8ecb7ecffeb64
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
> ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
> ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52427/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by Zoltan Haindrich <ki...@rxd.hu>.
> On Sept. 30, 2016, 5:30 p.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 434
> > <https://reviews.apache.org/r/52427/diff/1/?file=1516884#file1516884line434>
> >
> > Could u test if the precedence is correct w.r.t the other DBMS? If you put it here, it will be lower than */-, etc.
>
> pengcheng xiong wrote:
> Sorry, it should be "it will be the same as +- and lower than */, etc
putting it at the same level...is oracle-ish ;)
i've done some investigation about this...and i've changed my mind about its precedence ;)
patch#2 changes this: its below the | / & bitwise operators; and above the regular ops...i think nobody want to do math with stringconcatenated numbers
details are in the ticket:
https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
> On Sept. 30, 2016, 5:30 p.m., pengcheng xiong wrote:
> > ql/src/test/queries/clientpositive/concat_op.q, line 1
> > <https://reviews.apache.org/r/52427/diff/1/?file=1516886#file1516886line1>
> >
> > Could you add more tests? For example, create views, joins, gby, union, || with other operators, esp, |, *, +.
i've added some more testcases; it seems they work as expected
- Zoltan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/#review151046
-----------------------------------------------------------
On Oct. 13, 2016, 7:25 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52427/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 7:25 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and pengcheng xiong.
>
>
> Bugs: HIVE-14580
> https://issues.apache.org/jira/browse/HIVE-14580
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> adds support for || text concatenation
>
> * reuses the concat udf for doing the real work
> * same precedence as + / - ; I think this is expected
> * small ast rewrite code in SemanticAnalyzer
> * details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 025ea10cda5b126f46226809884088caadc0a3d0
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13f68796e4fe262098eaf6c5f09bc7f963d0f8cf
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
> ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
> ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52427/diff/
>
>
> Testing
> -------
>
> added qtest
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by pengcheng xiong <px...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/#review151046
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g (line 434)
<https://reviews.apache.org/r/52427/#comment219166>
Could u test if the precedence is correct w.r.t the other DBMS? If you put it here, it will be lower than */-, etc.
ql/src/test/queries/clientpositive/concat_op.q (line 1)
<https://reviews.apache.org/r/52427/#comment219167>
Could you add more tests? For example, create views, joins, gby, union, || with other operators, esp, |, *, +.
- pengcheng xiong
On Sept. 30, 2016, 3:08 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52427/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2016, 3:08 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and pengcheng xiong.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> adds support for || text concatenation
>
> * reuses the concat udf for doing the real work
> * same precedence as + / - ; I think this is expected
> * small ast rewrite code in SemanticAnalyzer
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g b623187a788a23eceb70a71de62671ad778a24b7
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g e6b70a0a0e328a27a808032857b8ecb7ecffeb64
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
> ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
> ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52427/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by Zoltan Haindrich <ki...@rxd.hu>.
> On Oct. 18, 2016, 6:51 a.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 445
> > <https://reviews.apache.org/r/52427/diff/3/?file=1535054#file1535054line445>
> >
> > change this to
> > {code}
> > precedenceConcatenateExpression
> > :
> > (precedencePlusExpression -> precedencePlusExpression)
> > (
> > precedenceConcatenateOperator plus=precedencePlusExpression
> > -> ^(TOK_FUNCTION {adaptor.create(Identifier, "concat")} {$precedenceConcatenateExpression.tree} $plus)
> > )*
> > -> {$precedenceConcatenateExpression.tree}
> > ;
> > {code}
> > So that we can finish the rewriting in the parser.
i've experimented with the same thing...but I was unable to put it together ;)
thank you for showing it, its far better than that ugly ast rewrite...
> On Oct. 18, 2016, 6:51 a.m., pengcheng xiong wrote:
> > ql/src/test/queries/clientpositive/concat_op.q, line 3
> > <https://reviews.apache.org/r/52427/diff/3/?file=1535056#file1535056line3>
> >
> > Please confirm that Postgres and/Oracle returns the same result.
If the question is about and/or precedence vs other databases: then yes, hive is consistent with them.
I've added a more complete check of the same thing at the end of the qfile - I just wanted to check it; and didn't notice it being left in the qfile...but it should never break...so I guess its better to add it.
- Zoltan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/#review153040
-----------------------------------------------------------
On Oct. 18, 2016, 3:06 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52427/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 3:06 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and pengcheng xiong.
>
>
> Bugs: HIVE-14580
> https://issues.apache.org/jira/browse/HIVE-14580
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> adds support for || text concatenation
>
> * reuses the concat udf for doing the real work
> * same precedence as + / - ; I think this is expected
> * small ast rewrite code in SemanticAnalyzer
> * details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 5d3fa6aeea5251e461c4b0a8277b4ff1b55f9115
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 50987c391e435f4e3c71243e90af89fc52f380aa
> ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
> ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52427/diff/
>
>
> Testing
> -------
>
> added qtest
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by pengcheng xiong <px...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/#review153040
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g (line 445)
<https://reviews.apache.org/r/52427/#comment222302>
change this to
{code}
precedenceConcatenateExpression
:
(precedencePlusExpression -> precedencePlusExpression)
(
precedenceConcatenateOperator plus=precedencePlusExpression
-> ^(TOK_FUNCTION {adaptor.create(Identifier, "concat")} {$precedenceConcatenateExpression.tree} $plus)
)*
-> {$precedenceConcatenateExpression.tree}
;
{code}
So that we can finish the rewriting in the parser.
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java (line 10685)
<https://reviews.apache.org/r/52427/#comment222303>
remove all the changes in SemanticAnalyzer.
ql/src/test/queries/clientpositive/concat_op.q (line 3)
<https://reviews.apache.org/r/52427/#comment222304>
Please confirm that Postgres and/Oracle returns the same result.
- pengcheng xiong
On Oct. 13, 2016, 7:25 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52427/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 7:25 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and pengcheng xiong.
>
>
> Bugs: HIVE-14580
> https://issues.apache.org/jira/browse/HIVE-14580
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> adds support for || text concatenation
>
> * reuses the concat udf for doing the real work
> * same precedence as + / - ; I think this is expected
> * small ast rewrite code in SemanticAnalyzer
> * details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 025ea10cda5b126f46226809884088caadc0a3d0
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13f68796e4fe262098eaf6c5f09bc7f963d0f8cf
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
> ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
> ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52427/diff/
>
>
> Testing
> -------
>
> added qtest
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/
-----------------------------------------------------------
(Updated Oct. 18, 2016, 3:06 p.m.)
Review request for hive, Ashutosh Chauhan and pengcheng xiong.
Changes
-------
parser-only change ;)
Bugs: HIVE-14580
https://issues.apache.org/jira/browse/HIVE-14580
Repository: hive-git
Description
-------
adds support for || text concatenation
* reuses the concat udf for doing the real work
* same precedence as + / - ; I think this is expected
* small ast rewrite code in SemanticAnalyzer
* details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 5d3fa6aeea5251e461c4b0a8277b4ff1b55f9115
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 50987c391e435f4e3c71243e90af89fc52f380aa
ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/52427/diff/
Testing
-------
added qtest
Thanks,
Zoltan Haindrich
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/
-----------------------------------------------------------
(Updated Oct. 13, 2016, 7:25 p.m.)
Review request for hive, Ashutosh Chauhan and pengcheng xiong.
Changes
-------
added some more tests...group by / order by
Bugs: HIVE-14580
https://issues.apache.org/jira/browse/HIVE-14580
Repository: hive-git
Description
-------
adds support for || text concatenation
* reuses the concat udf for doing the real work
* same precedence as + / - ; I think this is expected
* small ast rewrite code in SemanticAnalyzer
* details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 025ea10cda5b126f46226809884088caadc0a3d0
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13f68796e4fe262098eaf6c5f09bc7f963d0f8cf
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/52427/diff/
Testing
-------
added qtest
Thanks,
Zoltan Haindrich
Re: Review Request 52427: HIVE-14580 - Introduce || operator
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52427/
-----------------------------------------------------------
(Updated Oct. 13, 2016, 1:35 p.m.)
Review request for hive, Ashutosh Chauhan and pengcheng xiong.
Changes
-------
change operator precedence; new testcases
Bugs: HIVE-14580
https://issues.apache.org/jira/browse/HIVE-14580
Repository: hive-git
Description (updated)
-------
adds support for || text concatenation
* reuses the concat udf for doing the real work
* same precedence as + / - ; I think this is expected
* small ast rewrite code in SemanticAnalyzer
* details about other db || operators: https://issues.apache.org/jira/browse/HIVE-14580?focusedCommentId=15546578&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15546578
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 025ea10cda5b126f46226809884088caadc0a3d0
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13f68796e4fe262098eaf6c5f09bc7f963d0f8cf
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f3876b76de195fea78d87bfd98bd0ec2b87a0
ql/src/test/queries/clientpositive/concat_op.q PRE-CREATION
ql/src/test/results/clientpositive/concat_op.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/52427/diff/
Testing (updated)
-------
added qtest
Thanks,
Zoltan Haindrich