jackye1995 commented on code in PR #6637:
URL: https://github.com/apache/iceberg/pull/6637#discussion_r1083330622
##########
spark/v3.3/spark-extensions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4:
##########
@@ -168,34 +169,61 @@ fieldList
;
nonReserved
- : ADD | ALTER | AS | ASC | BY | CALL | DESC | DROP | FIELD | FIRST | LAST
| NULLS | ORDERED | PARTITION | TABLE | WRITE
- | DISTRIBUTED | LOCALLY | UNORDERED | REPLACE | WITH | IDENTIFIER_KW |
FIELDS | SET
- | TRUE | FALSE
+ : ADD | ALTER | AS | ASC | BY | CALL | CREATE | DAYS | DESC | DROP | FIELD
| FIRST | HOURS | LAST | NULLS | OF | ORDERED | PARTITION | TABLE | WRITE
+ | DISTRIBUTED | LOCALLY | MINUTES | UNORDERED | REPLACE | VERSION | WITH |
IDENTIFIER_KW | FIELDS | SET
+ | TAG | TRUE | FALSE
| MAP
;
+snapshotId
+ : number
+ ;
+
+snapshotRefRetain
Review Comment:
I overlooked this in the last PR, do we really need this extra definition?
To me, it feels more intuitive to just say `RETAIN number timeUnit` instead
of `RETAIN snapshotRefRetain snapshotRefRetainTimeUnit`
Another thing I overlooked is that looks like the Antlr convention is to use
all capital letters for these definitions, like `TIME_UNIT` instead of
`timeUnit`. I don't know if they imply different functionalities though.
##########
spark/v3.3/spark-extensions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4:
##########
@@ -73,6 +73,7 @@ statement
| ALTER TABLE multipartIdentifier WRITE writeSpec
#setWriteDistributionAndOrdering
| ALTER TABLE multipartIdentifier SET IDENTIFIER_KW FIELDS fieldList
#setIdentifierFields
| ALTER TABLE multipartIdentifier DROP IDENTIFIER_KW FIELDS fieldList
#dropIdentifierFields
+ | ALTER TABLE multipartIdentifier CREATE TAG identifier (AS OF VERSION
snapshotId)? (RETAIN snapshotRefRetain snapshotRefRetainTimeUnit)? #createTag
Review Comment:
I am debating with myself, if we should merge this case with the CREATE
BRANCH case or not.
I guess this question will also come for REPLACE BRANCH.
Currently leaning towards keep it as is so it is more clear, each extension
just does 1 exact thing, similar to we have 2 different actions for SET/DROP
identifier field, 3 for ADD/DROP/REPLACE partition field.
Any thoughts? @amogh-jahagirdar @hililiwei
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
