[GitHub] [ignite-3] korlov42 commented on a diff in pull request #1521: IGNITE-18254: Sql. Extend SQL grammar with ALTER ZONE statement

[GitBox <gi...@apache.org>]

korlov42 commented on code in PR #1521:
URL: https://github.com/apache/ignite-3/pull/1521#discussion_r1071215818


##########
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -532,3 +532,96 @@ SqlDrop SqlDropZone(Span s, boolean replace) :
         return new IgniteSqlDropZone(s.end(this), ifExists, zoneId);
     }
 }
+
+SqlNode SqlAlterZone() :
+{
+    final Span s;
+    final SqlIdentifier zoneId;
+    final SqlIdentifier newZoneId;
+    SqlNodeList optionList = null;
+}
+{
+    <ALTER> { s = span(); }
+    <ZONE> zoneId = CompoundIdentifier()

Review Comment:
   we need to support `ALTER ZONE IF EXISTS` syntax



##########
modules/sql-engine/src/main/codegen/config.fmpp:
##########
@@ -594,7 +598,8 @@ data: {
     # Return type of method implementation should be 'SqlNode'.
     # Example: "SqlShowDatabases()", "SqlShowTables()".
     statementParserMethods: [
-      "SqlAlterTable()"
+      "SqlAlterTable()",
+      "SqlAlterZone()"

Review Comment:
   since you've extended SqlAlter, why don't you put this in `alterStatementParserMethods`? 



##########
modules/sql-engine/src/main/codegen/config.fmpp:
##########
@@ -29,6 +29,7 @@ data: {
     imports: [
       "org.apache.calcite.sql.SqlCreate",
       "org.apache.calcite.sql.SqlDrop",
+      "org.apache.calcite.sql.SqlDdl",

Review Comment:
   why do we need this import? 



##########
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -532,3 +532,96 @@ SqlDrop SqlDropZone(Span s, boolean replace) :
         return new IgniteSqlDropZone(s.end(this), ifExists, zoneId);
     }
 }
+
+SqlNode SqlAlterZone() :
+{
+    final Span s;
+    final SqlIdentifier zoneId;
+    final SqlIdentifier newZoneId;
+    SqlNodeList optionList = null;
+}
+{
+    <ALTER> { s = span(); }
+    <ZONE> zoneId = CompoundIdentifier()
+    (
+      <RENAME> <TO> newZoneId = CompoundIdentifier() {

Review Comment:
   newZoneId have to be SimpleIdentifier. We can't move a zone to another schema by simply renaming it



##########
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -532,3 +532,96 @@ SqlDrop SqlDropZone(Span s, boolean replace) :
         return new IgniteSqlDropZone(s.end(this), ifExists, zoneId);
     }
 }
+
+SqlNode SqlAlterZone() :
+{
+    final Span s;
+    final SqlIdentifier zoneId;
+    final SqlIdentifier newZoneId;
+    SqlNodeList optionList = null;
+}
+{
+    <ALTER> { s = span(); }
+    <ZONE> zoneId = CompoundIdentifier()
+    (
+      <RENAME> <TO> newZoneId = CompoundIdentifier() {
+        return new IgniteSqlAlterZoneRenameTo(s.end(this), zoneId, newZoneId);
+      }
+      |
+      <WITH> { s.add(this); } optionList = AlterZoneOptions() {
+        return new IgniteSqlAlterZoneWith(s.end(this), zoneId, optionList);
+      }
+    )
+}
+
+SqlNodeList AlterZoneOptions() :
+{
+  List<SqlNode> list = new ArrayList<SqlNode>();
+  final Span s = Span.of();
+}
+{
+  AlterZoneOption(list)
+  (
+      <COMMA> { s.add(this); } AlterZoneOption(list)
+  )+

Review Comment:
   did you mean `*`? With current rule it's impossible to change exactly one param. Also, we need to cover this case in tests too



##########
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -532,3 +532,96 @@ SqlDrop SqlDropZone(Span s, boolean replace) :
         return new IgniteSqlDropZone(s.end(this), ifExists, zoneId);
     }
 }
+
+SqlNode SqlAlterZone() :
+{
+    final Span s;
+    final SqlIdentifier zoneId;
+    final SqlIdentifier newZoneId;
+    SqlNodeList optionList = null;
+}
+{
+    <ALTER> { s = span(); }
+    <ZONE> zoneId = CompoundIdentifier()
+    (
+      <RENAME> <TO> newZoneId = CompoundIdentifier() {
+        return new IgniteSqlAlterZoneRenameTo(s.end(this), zoneId, newZoneId);
+      }
+      |
+      <WITH> { s.add(this); } optionList = AlterZoneOptions() {

Review Comment:
   IMO, we should use `WITH` keyword to provide context of the operation. To alter a certain property `SET` seems more natural to me. WDYT?



-- 
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: notifications-unsubscribe@ignite.apache.org

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