[GitHub] [ignite-3] korlov42 commented on a diff in pull request #1872: IGNITE-19002 Move dataStorage configuration to distribution zones

["korlov42 (via GitHub)" <gi...@apache.org>]

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


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/IgniteSqlZoneOptionEnum.java:
##########
@@ -40,5 +40,8 @@ public enum IgniteSqlZoneOptionEnum {
     DATA_NODES_AUTO_ADJUST_SCALE_UP,
 
     /** Data nodes scale down auto adjust timeout. */
-    DATA_NODES_AUTO_ADJUST_SCALE_DOWN
+    DATA_NODES_AUTO_ADJUST_SCALE_DOWN,
+
+    /** Data storage engine. */
+    DATA_STORAGE_ENGINE

Review Comment:
   Let's add this option to `org.apache.ignite.internal.sql.engine.sql.DistributionZoneSqlDdlParserTest#createZoneNoOptions`. Besides, since IgniteSqlZoneOption.key is now an abstract identifier, no need to maintain this enum anymore, because under `org.apache.ignite.internal.sql.engine.sql` we keep entities that is part of sql AST only.
   
   It may be moved inside or close to DdlSqlToCommandConverter for convenience though 



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/IgniteSqlZoneOption.java:
##########
@@ -37,13 +37,13 @@ public class IgniteSqlZoneOption extends SqlCall {
             new SqlSpecialOperator("ZoneOption", SqlKind.OTHER);
 
     /** Option key. */
-    private final SqlLiteral key;
+    private final SqlIdentifier key;
 
     /** Option value. */
     private final SqlNode value;
 
     /** Creates {@link IgniteSqlZoneOption}. */
-    public IgniteSqlZoneOption(SqlLiteral key, SqlNode value, SqlParserPos pos) {
+    public IgniteSqlZoneOption(SqlIdentifier key, SqlNode value, SqlParserPos pos) {
         super(pos);
 
         this.key = key;

Review Comment:
   let's add an assertion that key is a simple identifier



##########
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -473,67 +473,18 @@ SqlNodeList CreateZoneOptionList() :
 void CreateZoneOption(List<SqlNode> list) :
 {
     final Span s;
-    final SqlLiteral key;
+    final SqlIdentifier key;
     final SqlNode val;
 }
 {
-    (
-        (
-            key = CreateNumericZoneOptionKey() { s = span(); }
-            <EQ>
-            val = NumericLiteral()
-        )
-        |
-        (
-            key = CreateStringZoneOptionKey() { s = span(); }
-            <EQ>
-            val = StringLiteral()
-        )
-    )
+    key = SimpleIdentifier() { s = span(); }
+    <EQ>
+    val = Literal()
     {
         list.add(new IgniteSqlZoneOption(key, val, s.end(this)));
     }
 }
 
-SqlLiteral CreateNumericZoneOptionKey() :
-{
-}
-{
-    <PARTITIONS>

Review Comment:
   also we need to remove all this keywords from config, because we don't need them anymore (keywords here are words enclosed in diamonds: PARTITIONS, REPLICAS, etc; see collections `keywords` and `nonReservedKeywords` in `modules/sql-engine/src/main/codegen/config.fmpp`)



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