[GitHub] [ignite-3] korlov42 opened a new pull request, #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

korlov42 opened a new pull request, #2508:
URL: https://github.com/apache/ignite-3/pull/2508

   Thank you for submitting the pull request.
   
   To streamline the review process of the patch and ensure better code quality
   we ask both an author and a reviewer to verify the following:
   
   ### The Review Checklist
   - [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:  
   \- There is a single JIRA ticket related to the pull request.  
   \- The web-link to the pull request is attached to the JIRA ticket.  
   \- The JIRA ticket has the Patch Available state.  
   \- The description of the JIRA ticket explains WHAT was made, WHY and HOW.  
   \- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
   - [ ] **Design:** new code conforms with the design principles of the components it is added to.
   - [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
   - [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
   - [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
   
   ### Notes
   - [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)


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

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

AMashenkov commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1311332766


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.Objects.requireNonNullElse;
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    private CreateTableCommand(
+            String tableName,
+            String schemaName,
+            List<String> primaryKeyColumns,
+            List<String> colocationColumns,
+            List<ColumnParams> columns,
+            String zoneName
+    ) throws CatalogValidationException {
+        this.tableName = tableName;
+        this.schemaName = schemaName;
+        this.primaryKeyColumns = primaryKeyColumns;
+        this.colocationColumns = colocationColumns;
+        this.columns = columns;
+        this.zoneName = zoneName;
+
+        validate();
+    }
+
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, tableName);
+
+        CatalogZoneDescriptor zone = zoneOrThrow(catalog, zoneName);
+
+        int id = catalog.objectIdGenState();
+        int tableId = id++;
+        int pkIndexId = id++;
+
+        CatalogTableDescriptor table = new CatalogTableDescriptor(
+                tableId,
+                tableName,
+                zone.id(),
+                CatalogTableDescriptor.INITIAL_TABLE_VERSION,
+                columns.stream().map(CatalogUtils::fromParams).collect(toList()),
+                primaryKeyColumns,
+                colocationColumns
+        );
+
+        String indexName = tableName + "_PK";
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, indexName);
+
+        CatalogHashIndexDescriptor pkIndex = new CatalogHashIndexDescriptor(
+                pkIndexId,
+                indexName,
+                tableId,
+                true,
+                primaryKeyColumns
+        );
+
+        return List.of(
+                new NewTableEntry(table),
+                new NewIndexEntry(pkIndex),
+                new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(schemaName, "Name of the schema");
+        validateIdentifier(tableName, "Name of the table");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("Table 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(format("Column with name '{}' specified more than once", column.name()));
+            }
+        }
+
+        if (nullOrEmpty(primaryKeyColumns)) {
+            throw new CatalogValidationException("Table should have primary key");
+        }
+
+        Set<String> primaryKeyColumnsSet = new HashSet<>();
+
+        for (String name : primaryKeyColumns) {
+            if (!columnNames.contains(name)) {
+                throw new CatalogValidationException(format("PK column '{}' is not part of table", name));
+            }
+
+            if (!primaryKeyColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("PK column '{}' specified more that once", name));
+            }
+        }
+
+        if (nullOrEmpty(colocationColumns)) {
+            throw new CatalogValidationException("Colocation columns could not be empty");
+        }
+
+        Set<String> colocationColumnsSet = new HashSet<>();
+
+        for (String name : colocationColumns) {
+            if (!primaryKeyColumnsSet.contains(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' is not part of PK", name));
+            }
+
+            if (!colocationColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' specified more that once", name));
+            }
+        }
+    }
+
+    /**
+     * Implementation of {@link CreateTableCommandBuilder}.
+     */
+    public static class Builder implements CreateTableCommandBuilder {
+        private @Nullable List<ColumnParams> columns;
+
+        private @Nullable String schemaName;
+
+        private @Nullable String tableName;
+
+        private @Nullable List<String> primaryKeyColumns;
+
+        private @Nullable List<String> colocationColumns;
+
+        private @Nullable String zoneName;
+
+        @Override
+        public CreateTableCommandBuilder schemaName(String schemaName) {
+            this.schemaName = schemaName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder tableName(String tableName) {
+            this.tableName = tableName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+            this.columns = columns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+            this.primaryKeyColumns = primaryKeyColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+            this.colocationColumns = colocationColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder zone(String zoneName) {
+            this.zoneName = zoneName;
+
+            return this;
+        }
+
+        @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+        @Override
+        public CatalogCommand build() {
+            String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+            List<String> colocationColumns = this.colocationColumns != null
+                    ? this.colocationColumns
+                    : primaryKeyColumns;
+
+            return new CreateTableCommand(

Review Comment:
   Let's make a defensive copy for all the collections,
   and add a note to builder interface/class javadoc if the builder is reusable or not.



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.Objects.requireNonNullElse;
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    private CreateTableCommand(
+            String tableName,
+            String schemaName,
+            List<String> primaryKeyColumns,
+            List<String> colocationColumns,
+            List<ColumnParams> columns,
+            String zoneName
+    ) throws CatalogValidationException {
+        this.tableName = tableName;
+        this.schemaName = schemaName;
+        this.primaryKeyColumns = primaryKeyColumns;
+        this.colocationColumns = colocationColumns;
+        this.columns = columns;
+        this.zoneName = zoneName;
+
+        validate();
+    }
+
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, tableName);
+
+        CatalogZoneDescriptor zone = zoneOrThrow(catalog, zoneName);
+
+        int id = catalog.objectIdGenState();
+        int tableId = id++;
+        int pkIndexId = id++;
+
+        CatalogTableDescriptor table = new CatalogTableDescriptor(
+                tableId,
+                tableName,
+                zone.id(),
+                CatalogTableDescriptor.INITIAL_TABLE_VERSION,
+                columns.stream().map(CatalogUtils::fromParams).collect(toList()),
+                primaryKeyColumns,
+                colocationColumns
+        );
+
+        String indexName = tableName + "_PK";
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, indexName);
+
+        CatalogHashIndexDescriptor pkIndex = new CatalogHashIndexDescriptor(
+                pkIndexId,
+                indexName,
+                tableId,
+                true,
+                primaryKeyColumns
+        );
+
+        return List.of(
+                new NewTableEntry(table),
+                new NewIndexEntry(pkIndex),
+                new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(schemaName, "Name of the schema");
+        validateIdentifier(tableName, "Name of the table");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("Table 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(format("Column with name '{}' specified more than once", column.name()));
+            }
+        }
+
+        if (nullOrEmpty(primaryKeyColumns)) {
+            throw new CatalogValidationException("Table should have primary key");
+        }
+
+        Set<String> primaryKeyColumnsSet = new HashSet<>();
+
+        for (String name : primaryKeyColumns) {
+            if (!columnNames.contains(name)) {
+                throw new CatalogValidationException(format("PK column '{}' is not part of table", name));
+            }
+
+            if (!primaryKeyColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("PK column '{}' specified more that once", name));
+            }
+        }
+
+        if (nullOrEmpty(colocationColumns)) {
+            throw new CatalogValidationException("Colocation columns could not be empty");
+        }
+
+        Set<String> colocationColumnsSet = new HashSet<>();
+
+        for (String name : colocationColumns) {
+            if (!primaryKeyColumnsSet.contains(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' is not part of PK", name));
+            }
+
+            if (!colocationColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' specified more that once", name));
+            }
+        }
+    }
+
+    /**
+     * Implementation of {@link CreateTableCommandBuilder}.
+     */
+    public static class Builder implements CreateTableCommandBuilder {
+        private @Nullable List<ColumnParams> columns;
+
+        private @Nullable String schemaName;
+
+        private @Nullable String tableName;
+
+        private @Nullable List<String> primaryKeyColumns;
+
+        private @Nullable List<String> colocationColumns;
+
+        private @Nullable String zoneName;
+
+        @Override
+        public CreateTableCommandBuilder schemaName(String schemaName) {
+            this.schemaName = schemaName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder tableName(String tableName) {
+            this.tableName = tableName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+            this.columns = columns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+            this.primaryKeyColumns = primaryKeyColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+            this.colocationColumns = colocationColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder zone(String zoneName) {
+            this.zoneName = zoneName;
+
+            return this;
+        }
+
+        @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+        @Override
+        public CatalogCommand build() {
+            String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+            List<String> colocationColumns = this.colocationColumns != null
+                    ? this.colocationColumns
+                    : primaryKeyColumns;
+
+            return new CreateTableCommand(

Review Comment:
   Let's make a defensive copy for all the collections,
   and add a note to the builder interface/class javadoc if the builder is reusable or not.



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/BaseCatalogManagerTest.java:
##########
@@ -159,24 +158,19 @@ protected static CreateSortedIndexParams createSortedIndexParams(
         return createSortedIndexParams(indexName, false, indexColumns, columnsCollations);
     }
 
-    protected static CreateTableParams createTableParams(
+    protected static CatalogCommand createTableCommand(
+            CreateTableCommandBuilder builder,

Review Comment:
   make sense, fixed



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

[GitHub] [ignite-3] xtern commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

xtern commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1310025159


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    public CreateTableCommand(

Review Comment:
   We usually restrict the ability to create directly with the constructor those classes that the builder creates :roll_eyes: :thinking: 



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

[GitHub] [ignite-3] xtern commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

xtern commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1309154559


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/builders/CreateTableCommandBuilderImpl.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.builders;
+
+import static java.util.Objects.requireNonNullElse;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.commands.ColumnParams;
+import org.apache.ignite.internal.catalog.commands.CreateTableCommand;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Implementation of {@link CreateTableCommandBuilder}.
+ */
+public class CreateTableCommandBuilderImpl implements CreateTableCommandBuilder {
+    private @Nullable List<ColumnParams> columns;
+
+    private @Nullable String schemaName;
+
+    private @Nullable String tableName;
+
+    private @Nullable List<String> primaryKeyColumns;
+
+    private @Nullable List<String> colocationColumns;
+
+    private @Nullable String zoneName;
+
+    @Override
+    public CreateTableCommandBuilder schemaName(String schemaName) {
+        this.schemaName = schemaName;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder tableName(String tableName) {
+        this.tableName = tableName;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+        this.columns = columns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+        this.primaryKeyColumns = primaryKeyColumns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+        this.colocationColumns = colocationColumns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder zone(String zoneName) {
+        this.zoneName = zoneName;
+
+        return this;
+    }
+
+    @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+    @Override
+    public CatalogCommand build() {
+        String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+        if (colocationColumns == null) {
+            colocationColumns = primaryKeyColumns;

Review Comment:
   Can we reuse builder after `build()` has been called?
   ```
   CatalogCommand cmd1 = commandBuilder.primaryKeyColumns(List.of("a")). ... .build();
   
   CatalogCommand cmd2 = commandBuilder.primaryKeyColumns(List.of("a", "b")).build();
   ```
   If so, then it seems to me that it's better not to change the state on `build` invocation.



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogValidationException.java:
##########
@@ -26,6 +27,15 @@
 public class CatalogValidationException extends IgniteInternalException {
     private static final long serialVersionUID = 2692301541251354006L;
 
+    /**
+     * Constructor.
+     *
+     * @param message Error message.
+     */
+    public CatalogValidationException(String message) {
+        super(Common.INTERNAL_ERR, message);

Review Comment:
   It seems worth adding a comment why we don't care about the error code.
   
   p.s. I didn't find where these exceptions are now mapped to STMT_VALIDATION_ERR code.
   Maybe I didn't look well enough or it will be added in [1].
   
   [1] https://issues.apache.org/jira/browse/IGNITE-20149



##########
modules/catalog/src/testFixtures/java/org/apache/ignite/internal/catalog/BaseCatalogManagerTest.java:
##########
@@ -159,24 +158,19 @@ protected static CreateSortedIndexParams createSortedIndexParams(
         return createSortedIndexParams(indexName, false, indexColumns, columnsCollations);
     }
 
-    protected static CreateTableParams createTableParams(
+    protected static CatalogCommand createTableCommand(
+            CreateTableCommandBuilder builder,

Review Comment:
   Personally, I don't see the point in adding a builder as a parameter and writing 40+ times `manager.createTableCommandBuilder()`  just to keep this method static, but it's up to you.



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

[GitHub] [ignite-3] korlov42 merged pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

korlov42 merged PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508


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

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

AMashenkov commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1310004057


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManager.java:
##########
@@ -24,25 +24,35 @@
 import org.apache.ignite.internal.catalog.commands.AlterZoneParams;
 import org.apache.ignite.internal.catalog.commands.CreateHashIndexParams;
 import org.apache.ignite.internal.catalog.commands.CreateSortedIndexParams;
-import org.apache.ignite.internal.catalog.commands.CreateTableParams;
 import org.apache.ignite.internal.catalog.commands.CreateZoneParams;
 import org.apache.ignite.internal.catalog.commands.DropIndexParams;
 import org.apache.ignite.internal.catalog.commands.DropTableParams;
 import org.apache.ignite.internal.catalog.commands.DropZoneParams;
 import org.apache.ignite.internal.catalog.commands.RenameZoneParams;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
 import org.apache.ignite.internal.manager.IgniteComponent;
 
 /**
  * The catalog manager provides schema manipulation methods and is responsible for managing distributed operations.
  */
 public interface CatalogManager extends IgniteComponent, CatalogService {
     /**
-     * Creates new table.
+     * Executes given command.
      *
-     * @param params Parameters.
-     * @return Operation future.
+     * <p>Accepts only those commands provided by builders returned by this very {@link CatalogManager}.
+     * Otherwise will throw {@link IllegalArgumentException}.
+     *
+     * @param command Command to execute.
+     * @return Future representing result of execution.
+     * @throws IllegalArgumentException If given command was created not by this manager.
+     * @see #createTableCommandBuilder()
+     */
+    CompletableFuture<Void> execute(CatalogCommand command) throws IllegalArgumentException;
+
+    /**
+     * Returns builder to create a command to create a new table.
      */
-    CompletableFuture<Void> createTable(CreateTableParams params);
+    CreateTableCommandBuilder createTableCommandBuilder();

Review Comment:
   We use CatalogManager for creating command builder.
   Then we use CatalogManager again to apply the command, and expect the CatalogCommand is  an command instance, which is created with the builder.
   
   Looks. like we expose some internals to the outside.
   As we don't need a CatalogManager to create the builder, then maybe this method could be moved to a separate Factory class?



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogValidationException.java:
##########
@@ -26,6 +27,15 @@
 public class CatalogValidationException extends IgniteInternalException {
     private static final long serialVersionUID = 2692301541251354006L;
 
+    /**
+     * Constructor.
+     *
+     * @param message Error message.
+     */
+    public CatalogValidationException(String message) {
+        super(Common.INTERNAL_ERR, message);

Review Comment:
   decided to introduce new code for this



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogValidationException.java:
##########
@@ -26,6 +27,15 @@
 public class CatalogValidationException extends IgniteInternalException {
     private static final long serialVersionUID = 2692301541251354006L;
 
+    /**
+     * Constructor.
+     *
+     * @param message Error message.
+     */
+    public CatalogValidationException(String message) {
+        super(Common.INTERNAL_ERR, message);

Review Comment:
   new code has been added



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

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

AMashenkov commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1310007118


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogValidationException.java:
##########
@@ -26,6 +27,15 @@
 public class CatalogValidationException extends IgniteInternalException {
     private static final long serialVersionUID = 2692301541251354006L;
 
+    /**
+     * Constructor.
+     *
+     * @param message Error message.
+     */
+    public CatalogValidationException(String message) {
+        super(Common.INTERNAL_ERR, message);

Review Comment:
   Why don't introduce a special code for this or pass the code from child classes?



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

[GitHub] [ignite-3] lowka commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

lowka commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1308503331


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java:
##########
@@ -0,0 +1,421 @@
+/*
+ * 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 org.apache.ignite.sql.ColumnType.INT32;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.mock;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogManager;
+import org.apache.ignite.internal.catalog.CatalogManagerImpl;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.ClockWaiter;
+import org.apache.ignite.internal.catalog.UpdateProducer;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.UpdateLog;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify validation of {@link CreateTableCommand}.
+ */
+@SuppressWarnings("DataFlowIssue")
+public class CreateTableCommandValidationTest extends BaseIgniteAbstractTest {
+    private static final String SCHEMA_NAME = "PUBLIC";
+    private static final String ZONE_NAME = "DEFAULT";
+
+    private static final CatalogZoneDescriptor DEFAULT_ZONE = new CatalogZoneDescriptor(
+            0, ZONE_NAME, 1, -1, -1, -1, -1, "", null
+    );
+
+    private final CatalogManager manager = new CatalogManagerImpl(
+            mock(UpdateLog.class),
+            mock(ClockWaiter.class)
+    );
+
+    @Test
+    void schemaNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {

Review Comment:
   Maybe test cases can be parameterised with `{null, "", " ", " "}`  and this collection of string can be used in other test cases (that check that table name, column name can not be null) as well? 



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java:
##########
@@ -0,0 +1,421 @@
+/*
+ * 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 org.apache.ignite.sql.ColumnType.INT32;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.mock;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogManager;
+import org.apache.ignite.internal.catalog.CatalogManagerImpl;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.ClockWaiter;
+import org.apache.ignite.internal.catalog.UpdateProducer;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.UpdateLog;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify validation of {@link CreateTableCommand}.
+ */
+@SuppressWarnings("DataFlowIssue")
+public class CreateTableCommandValidationTest extends BaseIgniteAbstractTest {
+    private static final String SCHEMA_NAME = "PUBLIC";
+    private static final String ZONE_NAME = "DEFAULT";
+
+    private static final CatalogZoneDescriptor DEFAULT_ZONE = new CatalogZoneDescriptor(
+            0, ZONE_NAME, 1, -1, -1, -1, -1, "", null
+    );
+
+    private final CatalogManager manager = new CatalogManagerImpl(
+            mock(UpdateLog.class),
+            mock(ClockWaiter.class)
+    );
+
+    @Test
+    void schemaNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {
+            builder.schemaName(name);
+
+            assertThrows(
+                    builder::build,
+                    CatalogValidationException.class,
+                    "Name of the schema can't be null or blank"
+            );
+        }
+    }
+
+    @Test
+    void tableNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {
+            builder.tableName(name);
+
+            assertThrows(
+                    builder::build,
+                    CatalogValidationException.class,
+                    "Name of the table can't be null or blank"
+            );
+        }
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void tableShouldHaveAtLeastOneColumn() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (List<ColumnParams> columns : new List[] {null, List.of()}) {

Review Comment:
   fixed



##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java:
##########
@@ -0,0 +1,421 @@
+/*
+ * 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 org.apache.ignite.sql.ColumnType.INT32;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.mock;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogManager;
+import org.apache.ignite.internal.catalog.CatalogManagerImpl;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.ClockWaiter;
+import org.apache.ignite.internal.catalog.UpdateProducer;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.UpdateLog;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify validation of {@link CreateTableCommand}.
+ */
+@SuppressWarnings("DataFlowIssue")
+public class CreateTableCommandValidationTest extends BaseIgniteAbstractTest {
+    private static final String SCHEMA_NAME = "PUBLIC";
+    private static final String ZONE_NAME = "DEFAULT";
+
+    private static final CatalogZoneDescriptor DEFAULT_ZONE = new CatalogZoneDescriptor(
+            0, ZONE_NAME, 1, -1, -1, -1, -1, "", null
+    );
+
+    private final CatalogManager manager = new CatalogManagerImpl(
+            mock(UpdateLog.class),
+            mock(ClockWaiter.class)
+    );
+
+    @Test
+    void schemaNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {

Review Comment:
   fixed



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/builders/CreateTableCommandBuilderImpl.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.builders;
+
+import static java.util.Objects.requireNonNullElse;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.commands.ColumnParams;
+import org.apache.ignite.internal.catalog.commands.CreateTableCommand;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Implementation of {@link CreateTableCommandBuilder}.
+ */
+public class CreateTableCommandBuilderImpl implements CreateTableCommandBuilder {
+    private @Nullable List<ColumnParams> columns;
+
+    private @Nullable String schemaName;
+
+    private @Nullable String tableName;
+
+    private @Nullable List<String> primaryKeyColumns;
+
+    private @Nullable List<String> colocationColumns;
+
+    private @Nullable String zoneName;
+
+    @Override
+    public CreateTableCommandBuilder schemaName(String schemaName) {
+        this.schemaName = schemaName;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder tableName(String tableName) {
+        this.tableName = tableName;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+        this.columns = columns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+        this.primaryKeyColumns = primaryKeyColumns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+        this.colocationColumns = colocationColumns;
+
+        return this;
+    }
+
+    @Override
+    public CreateTableCommandBuilder zone(String zoneName) {
+        this.zoneName = zoneName;
+
+        return this;
+    }
+
+    @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+    @Override
+    public CatalogCommand build() {
+        String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+        if (colocationColumns == null) {
+            colocationColumns = primaryKeyColumns;

Review Comment:
   good catch! Fixed



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

[GitHub] [ignite-3] korlov42 closed pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

korlov42 closed pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself
URL: https://github.com/apache/ignite-3/pull/2508


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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManager.java:
##########
@@ -24,25 +24,35 @@
 import org.apache.ignite.internal.catalog.commands.AlterZoneParams;
 import org.apache.ignite.internal.catalog.commands.CreateHashIndexParams;
 import org.apache.ignite.internal.catalog.commands.CreateSortedIndexParams;
-import org.apache.ignite.internal.catalog.commands.CreateTableParams;
 import org.apache.ignite.internal.catalog.commands.CreateZoneParams;
 import org.apache.ignite.internal.catalog.commands.DropIndexParams;
 import org.apache.ignite.internal.catalog.commands.DropTableParams;
 import org.apache.ignite.internal.catalog.commands.DropZoneParams;
 import org.apache.ignite.internal.catalog.commands.RenameZoneParams;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
 import org.apache.ignite.internal.manager.IgniteComponent;
 
 /**
  * The catalog manager provides schema manipulation methods and is responsible for managing distributed operations.
  */
 public interface CatalogManager extends IgniteComponent, CatalogService {
     /**
-     * Creates new table.
+     * Executes given command.
      *
-     * @param params Parameters.
-     * @return Operation future.
+     * <p>Accepts only those commands provided by builders returned by this very {@link CatalogManager}.
+     * Otherwise will throw {@link IllegalArgumentException}.
+     *
+     * @param command Command to execute.
+     * @return Future representing result of execution.
+     * @throws IllegalArgumentException If given command was created not by this manager.
+     * @see #createTableCommandBuilder()
+     */
+    CompletableFuture<Void> execute(CatalogCommand command) throws IllegalArgumentException;
+
+    /**
+     * Returns builder to create a command to create a new table.
      */
-    CompletableFuture<Void> createTable(CreateTableParams params);
+    CreateTableCommandBuilder createTableCommandBuilder();

Review Comment:
   I agree that this looks rather strange. Besides, there is violation of Liskov Substitution Principle.
   
   Considering only single command execution case, we actually may move method `execute` to the interface CatalogCommand, making a command self-contained. But I did not came up with a good solution to implement batched command. No matter what you do, you always need a guarantee that the command was created through the right builder.
   
   One option is to introduce method `createBatch(List<Command> commands)` on a manager, another option is to introduce method `execute(List<CatalogCommand> commands)` on a manager. 
   
   I decided to go with second option. With that said, having method `execute` on a manager for single command make api more consistent.
   
   > maybe this method could be moved to a separate Factory class?
   
   that's possible, but what will we do if we need access to some dynamic defaults from configuration, for instance? I think, static factory methods for such kind of objects not the best choice.  



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    public CreateTableCommand(

Review Comment:
   there is validation in contractor, thus it should be safe to create object directly. But anyways, I've reworked this part a bit, now constructor is private, so there is only one option to create command now



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    public CreateTableCommand(

Review Comment:
   there is validation in constructor, thus it should be safe to create object directly. But anyways, I've reworked this part a bit, now constructor is private, so there is only one option to create command now



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

[GitHub] [ignite-3] lowka commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

lowka commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1308504896


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java:
##########
@@ -0,0 +1,421 @@
+/*
+ * 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 org.apache.ignite.sql.ColumnType.INT32;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.mock;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.CatalogManager;
+import org.apache.ignite.internal.catalog.CatalogManagerImpl;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.ClockWaiter;
+import org.apache.ignite.internal.catalog.UpdateProducer;
+import org.apache.ignite.internal.catalog.commands.builders.CreateTableCommandBuilder;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableColumnDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.UpdateLog;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify validation of {@link CreateTableCommand}.
+ */
+@SuppressWarnings("DataFlowIssue")
+public class CreateTableCommandValidationTest extends BaseIgniteAbstractTest {
+    private static final String SCHEMA_NAME = "PUBLIC";
+    private static final String ZONE_NAME = "DEFAULT";
+
+    private static final CatalogZoneDescriptor DEFAULT_ZONE = new CatalogZoneDescriptor(
+            0, ZONE_NAME, 1, -1, -1, -1, -1, "", null
+    );
+
+    private final CatalogManager manager = new CatalogManagerImpl(
+            mock(UpdateLog.class),
+            mock(ClockWaiter.class)
+    );
+
+    @Test
+    void schemaNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {
+            builder.schemaName(name);
+
+            assertThrows(
+                    builder::build,
+                    CatalogValidationException.class,
+                    "Name of the schema can't be null or blank"
+            );
+        }
+    }
+
+    @Test
+    void tableNameMustBeNullOrBlank() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (String name : new  String[] {null, "", " ", "  "}) {
+            builder.tableName(name);
+
+            assertThrows(
+                    builder::build,
+                    CatalogValidationException.class,
+                    "Name of the table can't be null or blank"
+            );
+        }
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    void tableShouldHaveAtLeastOneColumn() {
+        CreateTableCommandBuilder builder = manager.createTableCommandBuilder();
+
+        builder = fillProperties(builder);
+
+        for (List<ColumnParams> columns : new List[] {null, List.of()}) {

Review Comment:
   Maybe this test case can also be parameterised with `null, List.of()` and `tableShouldHaveAtLeastOnePrimaryKeyColumn` as well?



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

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

AMashenkov commented on code in PR #2508:
URL: https://github.com/apache/ignite-3/pull/2508#discussion_r1311332766


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.Objects.requireNonNullElse;
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    private CreateTableCommand(
+            String tableName,
+            String schemaName,
+            List<String> primaryKeyColumns,
+            List<String> colocationColumns,
+            List<ColumnParams> columns,
+            String zoneName
+    ) throws CatalogValidationException {
+        this.tableName = tableName;
+        this.schemaName = schemaName;
+        this.primaryKeyColumns = primaryKeyColumns;
+        this.colocationColumns = colocationColumns;
+        this.columns = columns;
+        this.zoneName = zoneName;
+
+        validate();
+    }
+
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, tableName);
+
+        CatalogZoneDescriptor zone = zoneOrThrow(catalog, zoneName);
+
+        int id = catalog.objectIdGenState();
+        int tableId = id++;
+        int pkIndexId = id++;
+
+        CatalogTableDescriptor table = new CatalogTableDescriptor(
+                tableId,
+                tableName,
+                zone.id(),
+                CatalogTableDescriptor.INITIAL_TABLE_VERSION,
+                columns.stream().map(CatalogUtils::fromParams).collect(toList()),
+                primaryKeyColumns,
+                colocationColumns
+        );
+
+        String indexName = tableName + "_PK";
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, indexName);
+
+        CatalogHashIndexDescriptor pkIndex = new CatalogHashIndexDescriptor(
+                pkIndexId,
+                indexName,
+                tableId,
+                true,
+                primaryKeyColumns
+        );
+
+        return List.of(
+                new NewTableEntry(table),
+                new NewIndexEntry(pkIndex),
+                new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(schemaName, "Name of the schema");
+        validateIdentifier(tableName, "Name of the table");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("Table 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(format("Column with name '{}' specified more than once", column.name()));
+            }
+        }
+
+        if (nullOrEmpty(primaryKeyColumns)) {
+            throw new CatalogValidationException("Table should have primary key");
+        }
+
+        Set<String> primaryKeyColumnsSet = new HashSet<>();
+
+        for (String name : primaryKeyColumns) {
+            if (!columnNames.contains(name)) {
+                throw new CatalogValidationException(format("PK column '{}' is not part of table", name));
+            }
+
+            if (!primaryKeyColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("PK column '{}' specified more that once", name));
+            }
+        }
+
+        if (nullOrEmpty(colocationColumns)) {
+            throw new CatalogValidationException("Colocation columns could not be empty");
+        }
+
+        Set<String> colocationColumnsSet = new HashSet<>();
+
+        for (String name : colocationColumns) {
+            if (!primaryKeyColumnsSet.contains(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' is not part of PK", name));
+            }
+
+            if (!colocationColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' specified more that once", name));
+            }
+        }
+    }
+
+    /**
+     * Implementation of {@link CreateTableCommandBuilder}.
+     */
+    public static class Builder implements CreateTableCommandBuilder {
+        private @Nullable List<ColumnParams> columns;
+
+        private @Nullable String schemaName;
+
+        private @Nullable String tableName;
+
+        private @Nullable List<String> primaryKeyColumns;
+
+        private @Nullable List<String> colocationColumns;
+
+        private @Nullable String zoneName;
+
+        @Override
+        public CreateTableCommandBuilder schemaName(String schemaName) {
+            this.schemaName = schemaName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder tableName(String tableName) {
+            this.tableName = tableName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+            this.columns = columns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+            this.primaryKeyColumns = primaryKeyColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+            this.colocationColumns = colocationColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder zone(String zoneName) {
+            this.zoneName = zoneName;
+
+            return this;
+        }
+
+        @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+        @Override
+        public CatalogCommand build() {
+            String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+            List<String> colocationColumns = this.colocationColumns != null
+                    ? this.colocationColumns
+                    : primaryKeyColumns;
+
+            return new CreateTableCommand(

Review Comment:
   Let's make a defensive copy for all the collections.



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

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2508: IGNITE-20285 Move handler of CreateTable command from CatalogManager to command itself

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

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


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java:
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.Objects.requireNonNullElse;
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.ensureNoTableOrIndexExistsWithGivenName;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateColumnParams;
+import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.schemaOrThrow;
+import static org.apache.ignite.internal.catalog.commands.CatalogUtils.zoneOrThrow;
+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.CatalogCommand;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.catalog.CatalogValidationException;
+import org.apache.ignite.internal.catalog.descriptors.CatalogHashIndexDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogSchemaDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogTableDescriptor;
+import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor;
+import org.apache.ignite.internal.catalog.storage.NewIndexEntry;
+import org.apache.ignite.internal.catalog.storage.NewTableEntry;
+import org.apache.ignite.internal.catalog.storage.ObjectIdGenUpdateEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A command that adds a new table to the catalog.
+ */
+public class CreateTableCommand extends AbstractCatalogCommand {
+    /** Schema name where this new table will be created. */
+    private final String schemaName;
+
+    /** Table name. */
+    private final String tableName;
+
+    /** Primary key columns. */
+    private final List<String> primaryKeyColumns;
+
+    /** Colocation columns. */
+    private final List<String> colocationColumns;
+
+    /** Columns. */
+    private final List<ColumnParams> columns;
+
+    /** Distribution zone name. */
+    private final String zoneName;
+
+    /**
+     * Constructs the object.
+     *
+     * @param tableName Name of the table to create. Should not be null or blank.
+     * @param schemaName Name of the schema to create table in. Should not be null or blank.
+     * @param primaryKeyColumns Name of columns which represent primary key.
+     *      Should be subset of columns in param `columns`.
+     * @param colocationColumns Name of the columns participating in distribution calculation.
+     *      Should be subset of the primary key columns.
+     * @param columns List of the columns containing by the table. There is should be at least one column.
+     * @param zoneName Name of the zone to create table in. Should not be null or blank.
+     * @throws CatalogValidationException if any of restrictions above is violated.
+     */
+    private CreateTableCommand(
+            String tableName,
+            String schemaName,
+            List<String> primaryKeyColumns,
+            List<String> colocationColumns,
+            List<ColumnParams> columns,
+            String zoneName
+    ) throws CatalogValidationException {
+        this.tableName = tableName;
+        this.schemaName = schemaName;
+        this.primaryKeyColumns = primaryKeyColumns;
+        this.colocationColumns = colocationColumns;
+        this.columns = columns;
+        this.zoneName = zoneName;
+
+        validate();
+    }
+
+    @Override
+    public List<UpdateEntry> get(Catalog catalog) {
+        CatalogSchemaDescriptor schema = schemaOrThrow(catalog, schemaName);
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, tableName);
+
+        CatalogZoneDescriptor zone = zoneOrThrow(catalog, zoneName);
+
+        int id = catalog.objectIdGenState();
+        int tableId = id++;
+        int pkIndexId = id++;
+
+        CatalogTableDescriptor table = new CatalogTableDescriptor(
+                tableId,
+                tableName,
+                zone.id(),
+                CatalogTableDescriptor.INITIAL_TABLE_VERSION,
+                columns.stream().map(CatalogUtils::fromParams).collect(toList()),
+                primaryKeyColumns,
+                colocationColumns
+        );
+
+        String indexName = tableName + "_PK";
+
+        ensureNoTableOrIndexExistsWithGivenName(schema, indexName);
+
+        CatalogHashIndexDescriptor pkIndex = new CatalogHashIndexDescriptor(
+                pkIndexId,
+                indexName,
+                tableId,
+                true,
+                primaryKeyColumns
+        );
+
+        return List.of(
+                new NewTableEntry(table),
+                new NewIndexEntry(pkIndex),
+                new ObjectIdGenUpdateEntry(id - catalog.objectIdGenState())
+        );
+    }
+
+    private void validate() {
+        validateIdentifier(schemaName, "Name of the schema");
+        validateIdentifier(tableName, "Name of the table");
+
+        if (nullOrEmpty(columns)) {
+            throw new CatalogValidationException("Table 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(format("Column with name '{}' specified more than once", column.name()));
+            }
+        }
+
+        if (nullOrEmpty(primaryKeyColumns)) {
+            throw new CatalogValidationException("Table should have primary key");
+        }
+
+        Set<String> primaryKeyColumnsSet = new HashSet<>();
+
+        for (String name : primaryKeyColumns) {
+            if (!columnNames.contains(name)) {
+                throw new CatalogValidationException(format("PK column '{}' is not part of table", name));
+            }
+
+            if (!primaryKeyColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("PK column '{}' specified more that once", name));
+            }
+        }
+
+        if (nullOrEmpty(colocationColumns)) {
+            throw new CatalogValidationException("Colocation columns could not be empty");
+        }
+
+        Set<String> colocationColumnsSet = new HashSet<>();
+
+        for (String name : colocationColumns) {
+            if (!primaryKeyColumnsSet.contains(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' is not part of PK", name));
+            }
+
+            if (!colocationColumnsSet.add(name)) {
+                throw new CatalogValidationException(format("Colocation column '{}' specified more that once", name));
+            }
+        }
+    }
+
+    /**
+     * Implementation of {@link CreateTableCommandBuilder}.
+     */
+    public static class Builder implements CreateTableCommandBuilder {
+        private @Nullable List<ColumnParams> columns;
+
+        private @Nullable String schemaName;
+
+        private @Nullable String tableName;
+
+        private @Nullable List<String> primaryKeyColumns;
+
+        private @Nullable List<String> colocationColumns;
+
+        private @Nullable String zoneName;
+
+        @Override
+        public CreateTableCommandBuilder schemaName(String schemaName) {
+            this.schemaName = schemaName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder tableName(String tableName) {
+            this.tableName = tableName;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder columns(List<ColumnParams> columns) {
+            this.columns = columns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder primaryKeyColumns(List<String> primaryKeyColumns) {
+            this.primaryKeyColumns = primaryKeyColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder colocationColumns(List<String> colocationColumns) {
+            this.colocationColumns = colocationColumns;
+
+            return this;
+        }
+
+        @Override
+        public CreateTableCommandBuilder zone(String zoneName) {
+            this.zoneName = zoneName;
+
+            return this;
+        }
+
+        @SuppressWarnings("DataFlowIssue") // params are validated in constructor
+        @Override
+        public CatalogCommand build() {
+            String zoneName = requireNonNullElse(this.zoneName, CatalogService.DEFAULT_ZONE_NAME);
+
+            List<String> colocationColumns = this.colocationColumns != null
+                    ? this.colocationColumns
+                    : primaryKeyColumns;
+
+            return new CreateTableCommand(

Review Comment:
   good catch! Fixed



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