[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2528: IGNITE-20017: Introduce new command to catalog to create system view.

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -260,6 +278,25 @@ public static boolean isSupportedColumnTypeChange(ColumnType source, ColumnType
         return supportedTransitions != null && supportedTransitions.contains(target);
     }
 
+    /**
+     * Returns a list of schemas, replacing any schema with {@code newSchema} if it's name is equal to {@code newSchema.name()}.

Review Comment:
   ```suggestion
        * Returns a list of schemas, replacing any schema with {@code newSchema} if its name equals to {@code newSchema.name()}.
   ```



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -247,6 +249,22 @@ public static CatalogTableColumnDescriptor fromParams(ColumnParams params) {
                 precision, scale, length, defaultValue);
     }
 
+    /**
+     * Creates a system view descriptor based on parameters of create view command..
+     *
+     * @param id system view id.
+     * @param params Parameters.
+     *
+     * @return System view descriptor.
+     */
+    public static CatalogSystemViewDescriptor fromParams(int id, CreateSystemViewCommand params) {

Review Comment:
   looks like this method is not used 



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateSystemViewCommandBuilder.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.catalog.commands;
+
+import java.util.List;
+
+/**
+ * A builder to construct create system view commands.
+ */
+public interface CreateSystemViewCommandBuilder {
+
+    /**
+     * Sets view name.
+     *
+     * @param name System view name.
+     * @return this.
+     */
+    CreateSystemViewCommandBuilder name(String name);
+

Review Comment:
   we need to specify type of the view as well (cluster or node)



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogObjectDescriptor.java:
##########
@@ -59,10 +59,11 @@ public String toString() {
     }
 
     /** Catalog object type. */
-    enum Type {
+    public enum Type {

Review Comment:
   why do you change this? 



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateSystemViewCommand.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.catalog.commands;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.util.CollectionUtils.nullOrEmpty;
+import static org.apache.ignite.lang.IgniteStringFormatter.format;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSystemViewDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewSystemViewEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.lang.ErrorGroups.Common;
+
+/**
+ * Create system view command - creates or replaces a system view.
+ *
+ * <p>If a system with with the given name does not exists, this command adds this system view to {@code SYSTEM} schema.
+ * If a system with with the given name exists, this command replaces the existing system view.
+ */
+public class CreateSystemViewCommand extends AbstractCatalogCommand {
+
+    private final String name;
+
+    private final List<ColumnParams> columns;
+
+    /**
+     * Constructor.
+     *
+     * @param name View name.
+     * @param columns List of view columns.
+     */
+    CreateSystemViewCommand(String name, List<ColumnParams> columns) {
+        this.name = name;
+        this.columns = columns;
+
+        validate();
+    }
+
+    /**
+     * Returns a view name.
+     *
+     * @return View name.
+     */
+    public String name() {
+        return name;
+    }
+
+    /**
+     * Returns a list of view columns.
+     *
+     * @return List of view columns.
+     */
+    public List<ColumnParams> columns() {
+        return columns;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        int id = catalog.objectIdGenState();
+
+        List<CatalogTableColumnDescriptor> viewColumns = columns.stream().map(CatalogUtils::fromParams).collect(toList());
+        CatalogSystemViewDescriptor descriptor = new CatalogSystemViewDescriptor(id, name, viewColumns);
+

Review Comment:
   we have to validate that there is no other relations with the same name. Besides, you need to amend CreateTable command as well



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -260,6 +278,25 @@ public static boolean isSupportedColumnTypeChange(ColumnType source, ColumnType
         return supportedTransitions != null && supportedTransitions.contains(target);
     }
 
+    /**
+     * Returns a list of schemas, replacing any schema with {@code newSchema} if it's name is equal to {@code newSchema.name()}.
+     *
+     * @param newSchema A schema.
+     * @param schemas A list of schemas.
+     * @return A List of schemas.
+     */
+    public static List<CatalogSchemaDescriptor> replaceSchema(CatalogSchemaDescriptor newSchema,
+            Collection<CatalogSchemaDescriptor> schemas) {
+
+        return schemas.stream().map(s -> {
+            if (Objects.equals(s.name(), newSchema.name())) {

Review Comment:
   why by name and not by id?



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateSystemViewCommand.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.catalog.commands;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.util.CollectionUtils.nullOrEmpty;
+import static org.apache.ignite.lang.IgniteStringFormatter.format;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSystemViewDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewSystemViewEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.lang.ErrorGroups.Common;
+
+/**
+ * Create system view command - creates or replaces a system view.
+ *
+ * <p>If a system with with the given name does not exists, this command adds this system view to {@code SYSTEM} schema.
+ * If a system with with the given name exists, this command replaces the existing system view.
+ */
+public class CreateSystemViewCommand extends AbstractCatalogCommand {
+
+    private final String name;
+
+    private final List<ColumnParams> columns;
+
+    /**
+     * Constructor.
+     *
+     * @param name View name.
+     * @param columns List of view columns.
+     */
+    CreateSystemViewCommand(String name, List<ColumnParams> columns) {
+        this.name = name;
+        this.columns = columns;
+
+        validate();
+    }
+
+    /**
+     * Returns a view name.
+     *
+     * @return View name.
+     */
+    public String name() {
+        return name;
+    }
+
+    /**
+     * Returns a list of view columns.
+     *
+     * @return List of view columns.
+     */
+    public List<ColumnParams> columns() {
+        return columns;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        int id = catalog.objectIdGenState();
+
+        List<CatalogTableColumnDescriptor> viewColumns = columns.stream().map(CatalogUtils::fromParams).collect(toList());
+        CatalogSystemViewDescriptor descriptor = new CatalogSystemViewDescriptor(id, name, viewColumns);
+
+        return List.of(
+                new NewSystemViewEntry(descriptor),
+                new ObjectIdGenUpdateEntry(1)
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(name, "Name of the system view");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("System view should have at least one column");
+        }
+
+        Set<String> columnNames = new HashSet<>();
+
+        for (ColumnParams column : columns) {
+            validateColumnParams(column);
+
+            if (!columnNames.add(column.name())) {
+                throw new CatalogValidationException(Common.INTERNAL_ERR,

Review Comment:
   it's better to throw Catalog.VALIDATION_ERR



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateSystemViewCommand.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.catalog.commands;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.util.CollectionUtils.nullOrEmpty;
+import static org.apache.ignite.lang.IgniteStringFormatter.format;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSystemViewDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewSystemViewEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.lang.ErrorGroups.Common;
+
+/**
+ * Create system view command - creates or replaces a system view.
+ *
+ * <p>If a system with with the given name does not exists, this command adds this system view to {@code SYSTEM} schema.
+ * If a system with with the given name exists, this command replaces the existing system view.
+ */
+public class CreateSystemViewCommand extends AbstractCatalogCommand {
+
+    private final String name;
+
+    private final List<ColumnParams> columns;
+
+    /**
+     * Constructor.
+     *
+     * @param name View name.
+     * @param columns List of view columns.
+     */
+    CreateSystemViewCommand(String name, List<ColumnParams> columns) {
+        this.name = name;
+        this.columns = columns;
+
+        validate();
+    }
+
+    /**
+     * Returns a view name.
+     *
+     * @return View name.
+     */
+    public String name() {
+        return name;
+    }
+
+    /**
+     * Returns a list of view columns.
+     *
+     * @return List of view columns.
+     */
+    public List<ColumnParams> columns() {
+        return columns;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        int id = catalog.objectIdGenState();
+
+        List<CatalogTableColumnDescriptor> viewColumns = columns.stream().map(CatalogUtils::fromParams).collect(toList());
+        CatalogSystemViewDescriptor descriptor = new CatalogSystemViewDescriptor(id, name, viewColumns);
+
+        return List.of(
+                new NewSystemViewEntry(descriptor),
+                new ObjectIdGenUpdateEntry(1)
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(name, "Name of the system view");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("System view should have at least one column");
+        }
+
+        Set<String> columnNames = new HashSet<>();
+
+        for (ColumnParams column : columns) {
+            validateColumnParams(column);
+
+            if (!columnNames.add(column.name())) {
+                throw new CatalogValidationException(Common.INTERNAL_ERR,
+                        format("Column with name '{}' specified more than once", column.name()));
+            }
+        }
+    }
+
+    /**
+     * Creates a builder to construct instances of create system view command.
+     *
+     * @return A builder to create instances of create system view command.
+     */
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    /**
+     * Implementation of {@link CreateSystemViewCommandBuilder}.
+     */
+    public static class Builder implements CreateSystemViewCommandBuilder {

Review Comment:
   may be private



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateSystemViewCommand.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.catalog.commands;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.util.CollectionUtils.nullOrEmpty;
+import static org.apache.ignite.lang.IgniteStringFormatter.format;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSystemViewDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewSystemViewEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.lang.ErrorGroups.Common;
+
+/**
+ * Create system view command - creates or replaces a system view.
+ *
+ * <p>If a system with with the given name does not exists, this command adds this system view to {@code SYSTEM} schema.
+ * If a system with with the given name exists, this command replaces the existing system view.
+ */
+public class CreateSystemViewCommand extends AbstractCatalogCommand {
+
+    private final String name;
+
+    private final List<ColumnParams> columns;
+
+    /**
+     * Constructor.
+     *
+     * @param name View name.
+     * @param columns List of view columns.
+     */
+    CreateSystemViewCommand(String name, List<ColumnParams> columns) {
+        this.name = name;
+        this.columns = columns;
+
+        validate();
+    }
+
+    /**
+     * Returns a view name.
+     *
+     * @return View name.
+     */
+    public String name() {
+        return name;
+    }
+
+    /**
+     * Returns a list of view columns.
+     *
+     * @return List of view columns.
+     */
+    public List<ColumnParams> columns() {
+        return columns;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        int id = catalog.objectIdGenState();
+
+        List<CatalogTableColumnDescriptor> viewColumns = columns.stream().map(CatalogUtils::fromParams).collect(toList());
+        CatalogSystemViewDescriptor descriptor = new CatalogSystemViewDescriptor(id, name, viewColumns);
+
+        return List.of(

Review Comment:
   we don't want to update catalog on every restart of the node, thus if given view already exists, command should be NO-OP



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