You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "AMashenkov (via GitHub)" <gi...@apache.org> on 2023/05/18 10:17:50 UTC
[GitHub] [ignite-3] AMashenkov opened a new pull request, #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
AMashenkov opened a new pull request, #2085:
URL: https://github.com/apache/ignite-3/pull/2085
https://issues.apache.org/jira/browse/IGNITE-19460
Implemented CREATE INDEX, DROP INDEX commands.
--
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 merged pull request #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov merged PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085
--
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 #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085#discussion_r1209536857
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/ColumnParams.java:
##########
@@ -17,21 +17,29 @@
package org.apache.ignite.internal.catalog.commands;
-import java.io.Serializable;
import java.util.Objects;
import org.apache.ignite.sql.ColumnType;
/** Defines a particular column within table. */
-public class ColumnParams implements Serializable {
- private static final long serialVersionUID = 5602599481844743521L;
+public class ColumnParams {
+ /** Creates parameters builder. */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /** Column name. */
+ private String name;
- private final String name;
+ /** Column type. */
+ private ColumnType type;
- private final ColumnType type;
+ /** Nullability flag. */
+ private boolean nullable;
- private final boolean nullable;
+ /** Column default value. */
+ private DefaultValue defaultValueDefinition;
- private final DefaultValue defaultValueDefinition;
+ private ColumnParams() {}
/** Creates a column definition. */
public ColumnParams(String name, ColumnType type, DefaultValue defaultValueDefinition, boolean nullable) {
Review Comment:
It's a bit confusing to see a public constructor with parameters and a builder for the same 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 #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085#discussion_r1211691085
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -214,6 +233,53 @@ public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
return failedFuture(new UnsupportedOperationException("Not implemented yet."));
}
+ /** {@inheritDoc} */
+ @Override
+ public CompletableFuture<Void> createIndex(CreateIndexParams params) {
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ if (schema.index(params.indexName()) != null) {
+ throw new IndexAlreadyExistsException(schemaName, params.indexName());
+ }
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ IndexDescriptor index = CatalogUtils.fromParams(catalog.objectIdGenState(), table.id(), params);
Review Comment:
what if table does not have specified column? or what if columns contains duplicates?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -41,6 +50,50 @@ public static TableDescriptor fromParams(int id, CreateTableParams params) {
);
}
+ /**
+ * Converts CreateIndex command params to descriptor.
+ *
+ * @param id Index id.
+ * @param tableId Table id.
+ * @param params Parameters.
+ * @return Index descriptor.
+ */
+ public static IndexDescriptor fromParams(int id, int tableId, CreateIndexParams params) {
+ params.columns().stream()
+ .filter(Predicate.not(new HashSet<>()::add))
+ .findAny()
+ .ifPresent(colName -> {
+ throw new IllegalArgumentException("Can't create index on duplicate columns: columnName=" + colName);
Review Comment:
there should be no validation in utility classes. There might be assertion to validate that certain invariants are held, but no for validation of user input.
Besides, we need to stop using for such kind of errors
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractIndexCommandParams.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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;
+
+/**
+ * Abstract index ddl command.
+ */
+public class AbstractIndexCommandParams implements DdlCommandParams {
+ /** Index name. */
+ protected String indexName;
+
+ /** Schema name where this new index will be created. */
+ protected String schema;
+
+ /** Unique index flag. */
+ protected boolean unique;
Review Comment:
why do we need this flag for DropIndexParams?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateIndexParams.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.commands;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.descriptors.ColumnCollation;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * CREATE INDEX statement.
+ */
+public class CreateIndexParams extends AbstractIndexCommandParams {
Review Comment:
let's introduce separate param classes for HASH and SORTED indexes
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractIndexCommandParams.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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;
+
+/**
+ * Abstract index ddl command.
+ */
+public class AbstractIndexCommandParams implements DdlCommandParams {
+ /** Index name. */
+ protected String indexName;
+
+ /** Schema name where this new index will be created. */
+ protected String schema;
+
+ /** Unique index flag. */
+ protected boolean unique;
+
+ /** Quietly ignore this command if index existence check failed. */
+ protected boolean ifIndexExists;
Review Comment:
this flag should be removed
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractIndexCommandParams.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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;
+
+/**
+ * Abstract index ddl command.
+ */
+public class AbstractIndexCommandParams implements DdlCommandParams {
+ /** Index name. */
+ protected String indexName;
+
+ /** Schema name where this new index will be created. */
+ protected String schema;
+
+ /** Unique index flag. */
+ protected boolean unique;
+
+ /** Quietly ignore this command if index existence check failed. */
+ protected boolean ifIndexExists;
+
+ /**
+ * Returns index simple name.
+ */
+ public String indexName() {
+ return indexName;
+ }
+
+ /**
+ * Returns schema name.
+ */
+ public String schemaName() {
+ return schema;
+ }
+
+ /**
+ * Returns {@code true} if index is unique, {@code false} otherwise.
+ */
+ public boolean isUnique() {
+ return unique;
+ }
+
+ /**
+ * Parameters builder.
+ */
+ protected abstract static class AbstractBuilder<ParamT extends AbstractIndexCommandParams, BuilderT> {
Review Comment:
let's implement a proper builder pattern
--
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 #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085#discussion_r1209124194
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/internal/InternalSchemaTest.java:
##########
@@ -31,13 +31,16 @@
import org.apache.ignite.sql.IgniteSql;
import org.apache.ignite.sql.ResultSet;
import org.apache.ignite.sql.Session;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
/** Tests for internal manipulations with schema. */
public class InternalSchemaTest extends ClusterPerClassIntegrationTest {
/**
* Checks that schema version is updated even if column names are intersected.
+ * TODO Drop this test, when schema will be moved from configuration to Catalog.
*/
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19460")
Review Comment:
what is wrong with this test?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/NewIndexEntry.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.storage;
+
+import org.apache.ignite.internal.catalog.descriptors.IndexDescriptor;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new index.
+ */
+public class NewIndexEntry implements UpdateEntry {
+ private static final long serialVersionUID = 6717363577013237711L;
+
+ private final IndexDescriptor descriptor;
+
+ /**
+ * Constructs the object.
+ *
+ * @param descriptor A descriptor of a index to add.
Review Comment:
```suggestion
* @param descriptor A descriptor of an index to add.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropIndexEntry.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage;
+
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes deletion of a index.
+ */
+public class DropIndexEntry implements UpdateEntry {
+ private static final long serialVersionUID = -604729846502020728L;
+
+ private final int indexId;
+
+ /**
+ * Constructs the object.
+ *
+ * @param indexId An id of a index to drop.
+ */
+ public DropIndexEntry(int indexId) {
+ this.indexId = indexId;
+ }
+
+ /** Returns an id of a index to drop. */
Review Comment:
```suggestion
/** Returns an id of an index to drop. */
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropIndexEntry.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage;
+
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes deletion of a index.
+ */
+public class DropIndexEntry implements UpdateEntry {
+ private static final long serialVersionUID = -604729846502020728L;
+
+ private final int indexId;
+
+ /**
+ * Constructs the object.
+ *
+ * @param indexId An id of a index to drop.
Review Comment:
```suggestion
* @param indexId An id of an index to drop.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropIndexEntry.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage;
+
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes deletion of a index.
Review Comment:
```suggestion
* Describes deletion of an index.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropColumnsEntry.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.storage;
+
+import java.util.Set;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new columns.
Review Comment:
it's more like `deletion of columns from table`
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/NewIndexEntry.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.storage;
+
+import org.apache.ignite.internal.catalog.descriptors.IndexDescriptor;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new index.
+ */
+public class NewIndexEntry implements UpdateEntry {
+ private static final long serialVersionUID = 6717363577013237711L;
+
+ private final IndexDescriptor descriptor;
+
+ /**
+ * Constructs the object.
+ *
+ * @param descriptor A descriptor of a index to add.
+ */
+ public NewIndexEntry(IndexDescriptor descriptor) {
+ this.descriptor = descriptor;
+ }
+
+ /** Returns descriptor of a index to add. */
Review Comment:
```suggestion
/** Returns descriptor of an index to add. */
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/NewColumnsEntry.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.storage;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.descriptors.TableColumnDescriptor;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new columns.
Review Comment:
```suggestion
* Describes addition of new columns.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/NewColumnsEntry.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.storage;
+
+import java.util.List;
+import org.apache.ignite.internal.catalog.descriptors.TableColumnDescriptor;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new columns.
+ */
+public class NewColumnsEntry implements UpdateEntry {
+ private static final long serialVersionUID = 2970125889493580121L;
+
+ private final int tableId;
+ private final List<TableColumnDescriptor> descriptors;
+
+ /**
+ * Constructs the object.
+ *
+ * @param tableId Table id.
+ * @param descriptors A descriptors of columns to add.
Review Comment:
```suggestion
* @param descriptors Descriptors of columns to add.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CatalogEvent.java:
##########
@@ -27,5 +27,17 @@ public enum CatalogEvent implements Event {
TABLE_CREATE,
/** This event is fired, when a table was dropped in Catalog. */
- TABLE_DROP
+ TABLE_DROP,
+
+ /** This event is fired, when a index was created in Catalog. */
+ INDEX_CREATE,
+
+ /** This event is fired, when a index was dropped in Catalog. */
+ INDEX_DROP,
+
+ /** This event is fired, when a column was added to a table. */
+ COLUMN_ADD,
+
+ /** This event is fired, when a index was dropped from a table. */
+ COLUMN_DROP
Review Comment:
wondering why do we need this two events?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/IndexDescriptor.java:
##########
@@ -29,21 +32,34 @@ public abstract class IndexDescriptor extends ObjectDescriptor {
private final int tableId;
/** Unique constraint flag. */
- private boolean unique;
+ private final boolean unique;
+
+ /** Index columns. */
+ private final List<String> columns;
/** Write only flag. {@code True} when index is building. */
private boolean writeOnly;
- IndexDescriptor(int id, String name, int tableId, boolean unique) {
+ IndexDescriptor(int id, String name, int tableId, List<String> columns, boolean unique) {
super(id, Type.INDEX, name);
this.tableId = tableId;
this.unique = unique;
+ this.columns = Objects.requireNonNull(columns, "columns");
+
+ if (Set.copyOf(this.columns).size() != this.columns.size()) {
+ throw new IllegalArgumentException("Indexed columns should be unique");
Review Comment:
I believe there is no place for validation like this in an object descriptor. Any validation that is based on business rules should take place in manager, command handler, etc
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/DropColumnsEntry.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.storage;
+
+import java.util.Set;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Describes addition of a new columns.
+ */
+public class DropColumnsEntry implements UpdateEntry {
+ private static final long serialVersionUID = 2970125889493580121L;
+
+ private final int tableId;
+ private final Set<String> columns;
+
+ /**
+ * Constructs the object.
+ *
+ * @param tableId Table id.
+ * @param columns A names of columns to drop.
Review Comment:
```suggestion
* @param columns Names of columns to drop.
```
--
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 #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085#discussion_r1214043694
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CatalogEvent.java:
##########
@@ -31,4 +31,10 @@ public enum CatalogEvent implements Event {
/** This event is fired, when a column was added to or dropped from a table. */
TABLE_ALTER,
+
+ /** This event is fired, when a index was created in Catalog. */
Review Comment:
```suggestion
/** This event is fired, when an index was created in Catalog. */
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManager.java:
##########
@@ -59,4 +62,28 @@ public interface CatalogManager extends IgniteComponent, CatalogService {
* @return Operation future.
*/
CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params);
+
+ /**
+ * Creates new sorted index.
+ *
+ * @param params Parameters.
+ * @return Operation future.
+ */
+ CompletableFuture<Void> createIndex(CreateSortedIndexParams params);
+
+ /**
+ * Creates new index.
Review Comment:
Creates new `hash` index?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CatalogEvent.java:
##########
@@ -31,4 +31,10 @@ public enum CatalogEvent implements Event {
/** This event is fired, when a column was added to or dropped from a table. */
TABLE_ALTER,
+
+ /** This event is fired, when a index was created in Catalog. */
+ INDEX_CREATE,
+
+ /** This event is fired, when a index was dropped in Catalog. */
Review Comment:
```suggestion
/** This event is fired, when an index was dropped in Catalog. */
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -101,6 +102,7 @@ protected IgniteTransactions igniteTx() {
return CLUSTER_NODES.get(0).transactions();
}
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19592")
Review Comment:
This ticket has been resolved already :thinking:
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/events/CreateIndexEventParameters.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.events;
+
+import org.apache.ignite.internal.catalog.descriptors.IndexDescriptor;
+
+/**
+ * Create index event parameters contains a index descriptor for a newly created index.
Review Comment:
```suggestion
* Create index event parameters contains an index descriptor for a newly created index.
```
:thinking: `...parameters contains` is also looks incorrect, but we seem to already have it in neighboring classes)
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java:
##########
@@ -119,6 +120,7 @@ protected IgniteTransactions igniteTx() {
return CLUSTER_NODES.get(0).transactions();
}
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19592")
Review Comment:
This ticket has been resolved already :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] korlov42 commented on a diff in pull request #2085: IGNITE-19460 Sql. Implement missed DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2085:
URL: https://github.com/apache/ignite-3/pull/2085#discussion_r1214368568
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java:
##########
@@ -41,6 +48,43 @@ public static TableDescriptor fromParams(int id, CreateTableParams params) {
);
}
+ /**
+ * Converts CreateIndex command params to hash index descriptor.
+ *
+ * @param id Index id.
+ * @param tableId Table id.
+ * @param params Parameters.
+ * @return Index descriptor.
+ */
+ public static IndexDescriptor fromParams(int id, int tableId, CreateHashIndexParams params) {
+ return new HashIndexDescriptor(id,
+ params.indexName(),
+ tableId,
+ true,
Review Comment:
this actually should be `false`. The only valid case of creation of unique hash index is implicit creation of primary key
--
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