You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korlov42 (via GitHub)" <gi...@apache.org> on 2023/01/24 15:19:18 UTC
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #1555: IGNITE-18271: Sql. Provide commands and handlers for alter distributed zones operation
korlov42 commented on code in PR #1555:
URL: https://github.com/apache/ignite-3/pull/1555#discussion_r1085452132
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -514,6 +539,59 @@ private CreateZoneCommand convertCreateZone(IgniteSqlCreateZone createZoneNode,
return createZoneCmd;
}
+
+ /**
+ * Converts the given IgniteSqlAlterZoneSet AST node to a AlterZoneCommand.
+ *
+ * @param alterZoneSet Root node of the given AST.
+ * @param ctx Planning context.
+ */
+ private DdlCommand convertAlterZoneSet(IgniteSqlAlterZoneSet alterZoneSet, PlanningContext ctx) {
+ AlterZoneSetCommand alterZoneCmd = new AlterZoneSetCommand();
+
+ alterZoneCmd.schemaName(deriveSchemaName(alterZoneSet.name(), ctx));
+ alterZoneCmd.zoneName(deriveObjectName(alterZoneSet.name(), ctx, "zoneName"));
+ alterZoneCmd.ifExists(alterZoneSet.ifExists());
+
+ Set<IgniteSqlZoneOptionEnum> knownOptionNames = EnumSet.allOf(IgniteSqlZoneOptionEnum.class);
+
+ for (SqlNode optionNode : alterZoneSet.alterOptionsList().getList()) {
+ IgniteSqlZoneOption option = (IgniteSqlZoneOption) optionNode;
+ IgniteSqlZoneOptionEnum optionName = option.key().symbolValue(IgniteSqlZoneOptionEnum.class);
+
+ if (!knownOptionNames.remove(optionName)) {
+ throw new IgniteException(QUERY_VALIDATION_ERR,
+ String.format("Duplicate DDL command option has been specified [option=%s, query=%s]", optionName, ctx.query()));
+ }
+
+ DdlOptionInfo<AlterZoneSetCommand, ?> zoneOptionInfo = alterZoneOptionInfos.get(optionName);
+
+ assert zoneOptionInfo != null : optionName;
+ assert option.value() instanceof SqlLiteral : option.value();
+
+ updateCommandOption("Zone", optionName, (SqlLiteral) option.value(), zoneOptionInfo, ctx.query(), alterZoneCmd);
+ }
+
+ return alterZoneCmd;
+ }
+
+ /**
+ * Converts the given IgniteSqlAlterZoneRenameTo AST node to a AlterZoneCommand.
+ *
+ * @param alterZoneRename Root node of the given AST.
+ * @param ctx Planning context.
+ */
+ private DdlCommand convertAlterZoneRename(IgniteSqlAlterZoneRenameTo alterZoneRename, PlanningContext ctx) {
+ AlterZoneRenameCommand cmd = new AlterZoneRenameCommand();
+
+ cmd.schemaName(deriveSchemaName(alterZoneRename.name(), ctx));
+ cmd.zoneName(deriveObjectName(alterZoneRename.name(), ctx, "zoneName"));
+ cmd.newZoneName(deriveObjectName(alterZoneRename.newName(), ctx, "zoneName"));
Review Comment:
`deriveObjectName` is for identifiers which may be complex, but new name have to be simple, and we know this for sure. Probably, it would be better to call `alterZoneRename.newName().getSimple()` explicitly
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationNodeRemovedException.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration.tree;
+
+/**
+ * This exception is thrown when an operation attempts to modify a node with a key that was removed.
+ */
+public class ConfigurationNodeRemovedException extends ConfigurationNodeModificationException {
+
+ private static final long serialVersionUID = 2493615156271124701L;
+
+ /**
+ * The constructor.
+ *
+ * @param key the key.
Review Comment:
```suggestion
* @param key the key.
```
here and above
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationNodeRemovedException.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration.tree;
+
+/**
+ * This exception is thrown when an operation attempts to modify a node with a key that was removed.
+ */
+public class ConfigurationNodeRemovedException extends ConfigurationNodeModificationException {
+
+ private static final long serialVersionUID = 2493615156271124701L;
+
+ /**
+ * The constructor.
+ *
+ * @param key the key.
+ */
+ public ConfigurationNodeRemovedException(String key) {
+ super("Named List element with key \"" + key + "\" has been removed");
Review Comment:
We have some [guideline](https://cwiki.apache.org/confluence/display/IGNITE/Java+Logging+Guidelines) about how to format a message for the logger, but I believe this does make sense to use this format and for exception's message as well
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -514,6 +539,59 @@ private CreateZoneCommand convertCreateZone(IgniteSqlCreateZone createZoneNode,
return createZoneCmd;
}
+
+ /**
+ * Converts the given IgniteSqlAlterZoneSet AST node to a AlterZoneCommand.
+ *
+ * @param alterZoneSet Root node of the given AST.
+ * @param ctx Planning context.
Review Comment:
```suggestion
* @param alterZoneSet Root node of the given AST.
* @param ctx Planning context.
```
--
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