Re: Review Request 52427: HIVE-14580 - Introduce || operator

[Zoltan Haindrich]

> On Oct. 18, 2016, 6:51 a.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 445
> > 
> >
> > 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
> > 
> >
> > 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

[Zoltan Haindrich]

---
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

[pengcheng xiong]

---
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)


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)


remove all the changes in SemanticAnalyzer.



ql/src/test/queries/clientpositive/concat_op.q (line 3)


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

[Zoltan Haindrich]

> On Sept. 30, 2016, 5:30 p.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 434
> > 
> >
> > 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
> > 
> >
> > 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

[Zoltan Haindrich]

---
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

[Zoltan Haindrich]

---
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

Re: Review Request 52427: HIVE-14580 - Introduce || operator

[pengcheng xiong]

> On Sept. 30, 2016, 5:30 p.m., pengcheng xiong wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 434
> > 
> >
> > 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

[pengcheng xiong]

---
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)


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)


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
> 
>