You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/08/17 10:12:58 UTC
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
rpuch commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947536562
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTupleParser.java:
##########
@@ -107,6 +107,13 @@ public boolean hasNullMap() {
return entryBase > BinaryTupleSchema.HEADER_SIZE;
}
+ /**
+ * Returns the content of this tuple as a byte buffer.
+ */
+ public ByteBuffer asByteBuffer() {
+ return buffer;
Review Comment:
'as'-methods are about converting 'this' object to another class (of similar nature). Here, it is not a conversion of the whole parser (and it seems weird to view a parser as a buffer, they are completely different entities), but it's just getting the underlying buffer.
So I suggest to rename the method to something like `getByteBuffer()`, `byteBuffer()` or something similar.
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/HashIndexStorage.java:
##########
@@ -17,21 +17,38 @@
package org.apache.ignite.internal.storage.index;
+import java.util.Collection;
import org.apache.ignite.internal.schema.BinaryTuple;
import org.apache.ignite.internal.storage.RowId;
/**
- * Temporary API for creating Index rows from a list of column values. All columns must be sorted according to the index columns order,
- * specified by the {@link SortedIndexDescriptor#indexColumns()}.
+ * Storage for a Hash Index.
+ *
+ * <p>This storage serves as an unordered mapping from a subset of a table's columns (a.k.a. index columns) to a set of {@link RowId}s
+ * from a {@link org.apache.ignite.internal.storage.MvPartitionStorage} from the same table.
Review Comment:
Does `HashIndexStorage` instance store row IDs belonging to exactly one partition of a table, or row IDs from all partitions are stored in the same `HashIndexStorage`? If the latter is true, then the javadoc probably needs to be modified a bit, because now it seems to say that the row IDs are from 'some' (i.e. just one) partition storage.
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/HashIndexStorage.java:
##########
@@ -17,21 +17,38 @@
package org.apache.ignite.internal.storage.index;
+import java.util.Collection;
import org.apache.ignite.internal.schema.BinaryTuple;
import org.apache.ignite.internal.storage.RowId;
/**
- * Temporary API for creating Index rows from a list of column values. All columns must be sorted according to the index columns order,
- * specified by the {@link SortedIndexDescriptor#indexColumns()}.
+ * Storage for a Hash Index.
+ *
+ * <p>This storage serves as an unordered mapping from a subset of a table's columns (a.k.a. index columns) to a set of {@link RowId}s
+ * from a {@link org.apache.ignite.internal.storage.MvPartitionStorage} from the same table.
Review Comment:
Also, it would be great to explicitly state whether the index storage is one per table partition or one per table
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/index/AbstractHashIndexStorageTest.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * 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.storage.index;
+
+import static org.apache.ignite.internal.schema.configuration.SchemaConfigurationConverter.convert;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.apache.ignite.schema.SchemaBuilders.column;
+import static org.apache.ignite.schema.SchemaBuilders.tableBuilder;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.empty;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.schemas.table.ConstantValueDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.FunctionCallDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.HashIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.NullValueDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfiguration;
+import org.apache.ignite.configuration.schemas.table.TableView;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.chm.TestConcurrentHashMapStorageEngine;
+import org.apache.ignite.internal.storage.chm.schema.TestConcurrentHashMapDataStorageConfigurationSchema;
+import org.apache.ignite.internal.storage.index.impl.BinaryTupleRowSerializer;
+import org.apache.ignite.schema.SchemaBuilders;
+import org.apache.ignite.schema.definition.ColumnDefinition;
+import org.apache.ignite.schema.definition.ColumnType;
+import org.apache.ignite.schema.definition.TableDefinition;
+import org.apache.ignite.schema.definition.index.HashIndexDefinition;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Base class for Hash Index storage tests.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public abstract class AbstractHashIndexStorageTest {
+ private static final String INT_COLUMN_NAME = "intVal";
+
+ private static final String STR_COLUMN_NAME = "strVal";
+
+ private TableConfiguration tableCfg;
+
+ private HashIndexStorage indexStorage;
+
+ private BinaryTupleRowSerializer serializer;
+
+ @BeforeEach
+ void setUp(@InjectConfiguration(
+ polymorphicExtensions = {
+ HashIndexConfigurationSchema.class,
+ TestConcurrentHashMapDataStorageConfigurationSchema.class,
+ ConstantValueDefaultConfigurationSchema.class,
+ FunctionCallDefaultConfigurationSchema.class,
+ NullValueDefaultConfigurationSchema.class
+ },
+ // This value only required for configuration validity, it's not used otherwise.
+ value = "mock.dataStorage.name = " + TestConcurrentHashMapStorageEngine.ENGINE_NAME
+ ) TableConfiguration tableCfg) {
+ createTestTable(tableCfg);
+
+ this.tableCfg = tableCfg;
+ this.indexStorage = createIndexStorage();
+ this.serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+ }
+
+ /**
+ * Configures a test table with columns of all supported types.
+ */
+ private static void createTestTable(TableConfiguration tableCfg) {
+ ColumnDefinition pkColumn = column("pk", ColumnType.INT32).asNullable(false).build();
+
+ ColumnDefinition[] allColumns = {
+ pkColumn,
+ column(INT_COLUMN_NAME, ColumnType.INT32).asNullable(true).build(),
+ column(STR_COLUMN_NAME, ColumnType.string()).asNullable(true).build()
+ };
+
+ TableDefinition tableDefinition = tableBuilder("test", "foo")
+ .columns(allColumns)
+ .withPrimaryKey(pkColumn.name())
+ .build();
+
+ CompletableFuture<Void> createTableFuture = tableCfg.change(cfg -> convert(tableDefinition, cfg));
+
+ assertThat(createTableFuture, willCompleteSuccessfully());
+ }
+
+ /**
+ * Creates a storage instance for testing.
+ */
+ protected abstract HashIndexStorage createIndexStorage(String name, TableView tableCfg);
+
+ /**
+ * Creates a Sorted Index using the given columns.
Review Comment:
```suggestion
* Creates a Hash Index using the given columns.
```
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/index/AbstractHashIndexStorageTest.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * 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.storage.index;
+
+import static org.apache.ignite.internal.schema.configuration.SchemaConfigurationConverter.convert;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.apache.ignite.schema.SchemaBuilders.column;
+import static org.apache.ignite.schema.SchemaBuilders.tableBuilder;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.empty;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.schemas.table.ConstantValueDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.FunctionCallDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.HashIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.NullValueDefaultConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfiguration;
+import org.apache.ignite.configuration.schemas.table.TableView;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.chm.TestConcurrentHashMapStorageEngine;
+import org.apache.ignite.internal.storage.chm.schema.TestConcurrentHashMapDataStorageConfigurationSchema;
+import org.apache.ignite.internal.storage.index.impl.BinaryTupleRowSerializer;
+import org.apache.ignite.schema.SchemaBuilders;
+import org.apache.ignite.schema.definition.ColumnDefinition;
+import org.apache.ignite.schema.definition.ColumnType;
+import org.apache.ignite.schema.definition.TableDefinition;
+import org.apache.ignite.schema.definition.index.HashIndexDefinition;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Base class for Hash Index storage tests.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public abstract class AbstractHashIndexStorageTest {
+ private static final String INT_COLUMN_NAME = "intVal";
+
+ private static final String STR_COLUMN_NAME = "strVal";
+
+ private TableConfiguration tableCfg;
+
+ private HashIndexStorage indexStorage;
+
+ private BinaryTupleRowSerializer serializer;
+
+ @BeforeEach
+ void setUp(@InjectConfiguration(
+ polymorphicExtensions = {
+ HashIndexConfigurationSchema.class,
+ TestConcurrentHashMapDataStorageConfigurationSchema.class,
+ ConstantValueDefaultConfigurationSchema.class,
+ FunctionCallDefaultConfigurationSchema.class,
+ NullValueDefaultConfigurationSchema.class
+ },
+ // This value only required for configuration validity, it's not used otherwise.
+ value = "mock.dataStorage.name = " + TestConcurrentHashMapStorageEngine.ENGINE_NAME
+ ) TableConfiguration tableCfg) {
+ createTestTable(tableCfg);
+
+ this.tableCfg = tableCfg;
+ this.indexStorage = createIndexStorage();
+ this.serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+ }
+
+ /**
+ * Configures a test table with columns of all supported types.
Review Comment:
Only columns of types `INT32` and `STRING` are created currently. Are those 'all' types we support?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexStorage.java:
##########
@@ -26,7 +26,7 @@
/**
* Storage for a Sorted Index.
*
- * <p>This storage serves as a sorted mapping from a subset of a table's columns (a.k.a. index columns) to a {@link RowId}
+ * <p>This storage serves as a sorted mapping from a subset of a table's columns (a.k.a. index columns) to a set of {@link RowId}s
* from a {@link org.apache.ignite.internal.storage.MvPartitionStorage} from the same table.
Review Comment:
I added a comment about same line on `HashIndexStorage`. If it is changed there, it should probably be changed here as well.
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/index/impl/TestHashIndexStorage.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.storage.index.impl;
+
+import static org.apache.ignite.internal.util.IgniteUtils.capacity;
+
+import java.nio.ByteBuffer;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import org.apache.ignite.internal.schema.BinaryTuple;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.index.HashIndexDescriptor;
+import org.apache.ignite.internal.storage.index.HashIndexStorage;
+import org.apache.ignite.internal.storage.index.IndexRow;
+
+/**
+ * Test-only implementation of a {@link HashIndexStorage}.
+ */
+public class TestHashIndexStorage implements HashIndexStorage {
+ private final ConcurrentMap<ByteBuffer, Set<RowId>> index;
+
+ private final HashIndexDescriptor descriptor;
+
+ /**
+ * Constructor.
+ */
+ public TestHashIndexStorage(HashIndexDescriptor descriptor) {
+ this.descriptor = descriptor;
+ this.index = new ConcurrentHashMap<>();
Review Comment:
Could we initialize this on the field declaration?
--
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