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/16 13:51:29 UTC
[GitHub] [ignite-3] sashapolo opened a new pull request, #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
sashapolo opened a new pull request, #1014:
URL: https://github.com/apache/ignite-3/pull/1014
https://issues.apache.org/jira/browse/IGNITE-17534
--
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] tkalkirill merged pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
tkalkirill merged PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014
--
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] rpuch commented on pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
rpuch commented on PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#issuecomment-1219089790
Tests have failed. This can be fixed by adding `UnlimitedBudgetConfigurationSchema.class` to the list of the polymorphic configurations.
--
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] rpuch commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947760809
##########
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:
I mean, the comment should match the code. If the code is fine, let's change the comment.
--
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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947745039
##########
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:
no. Do you suggest to create an index with all supported types? I'm not sure it's worth the complexity if we are talking about hash indices as they don't actually care about what's inside a BinaryTuple
--
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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947743531
##########
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:
All indices store data from a single partition. I will clarify
--
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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947816202
##########
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:
oh, that's a copy-paste mistake, will fix it, thank 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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r946909212
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -78,6 +79,16 @@ public interface MvTableStorage {
@Nullable
SortedIndexStorage getSortedIndex(int partitionId, String indexName);
+ /**
+ * Returns an already created Hash Index with the given name or {@code null} if it does not exist.
+ *
+ * @param partitionId Partition ID for which this index has been built.
+ * @param indexName Index name.
+ * @return Hash Index storage or {@code null} if it does not exist..
+ */
+ @Nullable
+ HashIndexStorage getHashIndex(int partitionId, String indexName);
Review Comment:
I personally don't like this formatting, sorry
--
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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r946908823
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -78,6 +79,16 @@ public interface MvTableStorage {
@Nullable
SortedIndexStorage getSortedIndex(int partitionId, String indexName);
+ /**
+ * Returns an already created Hash Index with the given name or {@code null} if it does not exist.
+ *
+ * @param partitionId Partition ID for which this index has been built.
+ * @param indexName Index name.
+ * @return Hash Index storage or {@code null} if it does not exist..
+ */
+ @Nullable
+ HashIndexStorage getHashIndex(int partitionId, String indexName);
Review Comment:
Why?
--
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] tkalkirill commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r946878752
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -426,6 +427,12 @@ public void createIndex(String indexName) {
throw new UnsupportedOperationException("Not implemented yet");
}
+ @Override
+ @Nullable
+ public HashIndexStorage getHashIndex(int partitionId, String indexName) {
+ throw new UnsupportedOperationException("Not implemented yet");
+ }
Review Comment:
```suggestion
@Override
public @Nullable HashIndexStorage getHashIndex(int partitionId, String indexName) {
throw new UnsupportedOperationException("Not implemented yet");
}
```
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -78,6 +79,16 @@ public interface MvTableStorage {
@Nullable
SortedIndexStorage getSortedIndex(int partitionId, String indexName);
+ /**
+ * Returns an already created Hash Index with the given name or {@code null} if it does not exist.
+ *
+ * @param partitionId Partition ID for which this index has been built.
+ * @param indexName Index name.
+ * @return Hash Index storage or {@code null} if it does not exist..
+ */
+ @Nullable
+ HashIndexStorage getHashIndex(int partitionId, String indexName);
Review Comment:
Maybe **TableIndexConfiguration** instead **indexName**?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -78,6 +79,16 @@ public interface MvTableStorage {
@Nullable
SortedIndexStorage getSortedIndex(int partitionId, String indexName);
+ /**
+ * Returns an already created Hash Index with the given name or {@code null} if it does not exist.
+ *
+ * @param partitionId Partition ID for which this index has been built.
+ * @param indexName Index name.
+ * @return Hash Index storage or {@code null} if it does not exist..
+ */
+ @Nullable
+ HashIndexStorage getHashIndex(int partitionId, String indexName);
Review Comment:
```suggestion
/**
* Returns an already created Hash Index with the given name or {@code null} if it does not exist.
*
* @param partitionId Partition ID for which this index has been built.
* @param indexName Index name.
* @return Hash Index storage or {@code null} if it does not exist.
*/
@Nullable HashIndexStorage getHashIndex(int partitionId, String indexName);
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -151,6 +152,12 @@ public void createIndex(String indexName) {
throw new UnsupportedOperationException("Not implemented yet");
}
+ @Override
+ @Nullable
+ public HashIndexStorage getHashIndex(int partitionId, String indexName) {
+ throw new UnsupportedOperationException("Not implemented yet");
+ }
+
Review Comment:
```suggestion
@Override
public @Nullable HashIndexStorage getHashIndex(int partitionId, String indexName) {
throw new UnsupportedOperationException("Not implemented yet");
}
```
--
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] sashapolo commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1014:
URL: https://github.com/apache/ignite-3/pull/1014#discussion_r947610799
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -151,6 +152,12 @@ public void createIndex(String indexName) {
throw new UnsupportedOperationException("Not implemented yet");
}
+ @Override
+ @Nullable
+ public HashIndexStorage getHashIndex(int partitionId, String indexName) {
+ throw new UnsupportedOperationException("Not implemented yet");
+ }
+
Review Comment:
Do we have such formatting in the code style? If no, I would prefer to leave it as-is
--
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] rpuch commented on a diff in pull request #1014: IGNITE-17534 Introduce HashIndexStorage interface and test implementation
Posted by GitBox <gi...@apache.org>.
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