[GitHub] [spark] carlfu-db commented on a diff in pull request #38404: [SPARK-40956] SQL Equivalent for Dataframe overwrite command

[GitBox]

carlfu-db commented on code in PR #38404:
URL: https://github.com/apache/spark/pull/38404#discussion_r1013470376


##########
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -319,7 +319,7 @@ query
 insertInto
     : INSERT OVERWRITE TABLE? multipartIdentifier (partitionSpec (IF NOT 
EXISTS)?)?  identifierList?        #insertOverwriteTable
     | INSERT INTO TABLE? multipartIdentifier partitionSpec? (IF NOT EXISTS)? 
identifierList?                #insertIntoTable
-    | INSERT INTO TABLE? multipartIdentifier REPLACE whereClause? 
identifierList?                            #insertIntoReplaceWhere
+    | INSERT INTO TABLE? multipartIdentifier REPLACE whereClause? 
identifierList                            #insertIntoReplaceWhere

Review Comment:
   This change can make the `SQLQueryTestSuite interval.sql` test success 
again, but it won't work for the current PR. cc @cloud-fan 



##########
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -319,7 +319,7 @@ query
 insertInto
     : INSERT OVERWRITE TABLE? multipartIdentifier (partitionSpec (IF NOT 
EXISTS)?)?  identifierList?        #insertOverwriteTable
     | INSERT INTO TABLE? multipartIdentifier partitionSpec? (IF NOT EXISTS)? 
identifierList?                #insertIntoTable
-    | INSERT INTO TABLE? multipartIdentifier REPLACE whereClause? 
identifierList?                            #insertIntoReplaceWhere
+    | INSERT INTO TABLE? multipartIdentifier REPLACE whereClause? 
identifierList                            #insertIntoReplaceWhere

Review Comment:
   <img width="1419" alt="image" 
src="https://user-images.githubusercontent.com/114777395/199851448-df273e7b-4a8b-4dc8-875f-b47882d8c265.png";>
   



##########
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -319,6 +319,7 @@ query
 insertInto
     : INSERT OVERWRITE TABLE? multipartIdentifier (partitionSpec (IF NOT 
EXISTS)?)?  identifierList?        #insertOverwriteTable
     | INSERT INTO TABLE? multipartIdentifier partitionSpec? (IF NOT EXISTS)? 
identifierList?                #insertIntoTable
+    | INSERT INTO TABLE? multipartIdentifier REPLACE whereClause? 
identifierList?                           #insertIntoReplaceWhere

Review Comment:
   Oh, that is actually something I asked during our last chat. I think I still 
have some confusion. 
   
   The grammar should be:
   ```
   INSERT INTO TABLE? multipartIdentifier REPLACE whereClause(required) 
identifierList(required) 
   ```
   as the unit test in this PR
   ```
   INSERT INTO testcat.tbl REPLACE WHERE id = 3 SELECT * FROM source2
   ```
   
   Following the notion that `? only means required` in the grammar, we should 
remove both `?` and the grammar becomes
   ```
   INSERT INTO TABLE? multipartIdentifier REPLACE whereClause identifierList
   ```
   
   However, this will make the unit test fail, as identifierList's `?` is 
removed
   <img width="1548" alt="image" 
src="https://user-images.githubusercontent.com/114777395/201408251-6d695422-f2d6-447e-9197-cd80c1b1e905.png";>
   
   
   



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -288,6 +289,11 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] 
with SQLConfHelper wit
           query,
           overwrite = true,
           ifPartitionNotExists)
+      case ctx: InsertIntoReplaceWhereContext =>

Review Comment:
   Hmm, not fully understand but I think it might related to my previous 
comment :) 



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

To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org

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