[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-20 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-661063885


   > The previous exception message doesn't particularly wrong to me if you're 
arguing:
   > 
   > ```
   > LATERAL cannot be used together with PIVOT in FROM clause
   > ```
   > 
   > doesn't explain the case below:
   > 
   > ```sql
   > SELECT * FROM person
   > PIVOT (
   > count(distinct age) as a
   > for name in ('Mary','John')
   > )
   > lateral view outer explode(array(30,60)) tabelName as c_age
   > lateral view explode(array(40,80)) as d_age
   > ```
   > 
   > because it already clarifies `FROM` and `PIVOT`.
   
   Now , i got it.
   
https://github.com/apache/spark/blob/db47c6e340a63100d7c0e85abf237adc4e2174cc/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4#L501-L502
   and 
   
https://github.com/apache/spark/blob/db47c6e340a63100d7c0e85abf237adc4e2174cc/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4#L574
   will allow the PR usage and disallow the defined Non-Commonality.
   Thanks @maropu @HyukjinKwon .closing ticket..




This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-20 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-660990164


   cc @maropu @HyukjinKwon, Could you please help check what else need i do to 
close this PR ?



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-16 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-659454251


   Thanks @maropu , jenkins please retest it .



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-16 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-659393021


   > I don't dig into it though, probably the second case is matched in 
`querySpecification`:
   > 
https://github.com/apache/spark/blob/db47c6e340a63100d7c0e85abf237adc4e2174cc/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4#L501-L502
   
   emmm,   if so, when our user execute the following sql and get expected 
result, they may think `PIVOT` Clause and work together with `lateral ` clause, 
we may make it more clear in SqlBase.g4 or error notification ? WDYT?
   ```
   SELECT * FROM person
   PIVOT (
   count(distinct age) as a
   for name in ('Mary','John')
   )
   lateral view outer explode(array(30,60)) tabelName as c_age
   lateral view explode(array(40,80)) as d_age
   ```
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-16 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-659373259


   > > We have the following defined in 
spark/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4.
 but it seems pivotClause and lateralView need to replace the order too?
   > 
   > Why did you think so? Seems like the definition make the error message 
explicit:
   > [#21324 
(comment)](https://github.com/apache/spark/pull/21324#discussion_r188078585)
   
   @maropu  Now my confusing point is the following sytax is not defined in 
SqlBase.g4. but it seems to be allowed to used in Spark, and no errors throw 
out. it might be  understandable if this behavior throws error like `LATERAL 
cannot be used together with PIVOT in FROM clause` or basic sytax error. 
   
   ```
   SELECT * FROM person
   PIVOT (
   count(distinct age) as a
   for name in ('Mary','John')
   )
   lateral view outer explode(array(30,60)) tabelName as c_age
   lateral view explode(array(40,80)) as d_age
   ```



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-16 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-659325584


   > The mssage update itself seems okay; cc: @maryannxue @gatorsmile cuz I 
checked the original PR for `PIVOT`: #21187
   
I just pull with latest code, it still behaviors the same as PR 
description.  any new ideas? @maropu @HyukjinKwon @maryannxue @gatorsmile 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] GuoPhilipse commented on pull request #29126: [SPARK-32324][SQL]Fix error messages during using PIVOT and lateral view

2020-07-16 Thread GitBox 


GuoPhilipse commented on pull request #29126:
URL: https://github.com/apache/spark/pull/29126#issuecomment-659322465


   We have the following defined in 
spark/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4.
  but it seems `pivotClause` and `lateralView` need to replace the order too?
   
   ```
   fromClause
   : FROM relation (',' relation)* lateralView* pivotClause?
   ```



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org