rpuch commented on code in PR #739: URL: https://github.com/apache/ignite-3/pull/739#discussion_r842605027
########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java: ########## @@ -0,0 +1,78 @@ +/* + * 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; + +import java.util.UUID; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Multi-versioned partition storage. + * POC version, that represents a combination between a replicated TX-aware MV storage and physical MV storage. Their API are very similar, + * although there are very important differences that will be addressed in the future. + */ +public interface MvPartitionStorage { + /** + * Reads the value from the storage as it was at the given timestamp. {@code null} timestamp means reading the latest value. + * + * @param key Key. + * @param timestamp Timestamp. + * @return Binary row that corresponds to the key or {@code null} if value is not found. + */ + @Nullable + BinaryRow read(BinaryRow key, @Nullable Timestamp timestamp); + + /** + * Creates an uncommitted version, assigned to the given transaction id. + * + * @param row Binary row to update. Key only row means value removal. + * @param txId Transaction id. + * @throws TxIdMismatchException If there's another pending update associated with different transaction id. + * @throws StorageException If failed to write data to the storage. + */ + void addWrite(BinaryRow row, UUID txId) throws TxIdMismatchException, StorageException; + + /** + * Aborts a pending update of the ongoing uncommitted transaction. Invoked during rollback. + * + * @param key Key. + * @throws StorageException If failed to write data to the storage. + */ + void abortWrite(BinaryRow key) throws StorageException; + + /** + * Commits a pending update of the ongoing transaction. Invoked during commit. Committed value will be versioned by the given timestamp. + * + * @param key Key. + * @param timestamp Timestamp to associate with committed value. + * @throws StorageException If failed to write data to the storage. + */ + void commitWrite(BinaryRow key, Timestamp timestamp) throws StorageException; + + /** + * Scans the partition and returns a cursor of values in at the given timestamp. Review Comment: 'in at' - there is probably a typo ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java: ########## @@ -0,0 +1,233 @@ +/* + * 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; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Comparator; +import java.util.List; +import java.util.UUID; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.junit.jupiter.api.Test; + +/** + * Base test for MV partition storages. + */ +public abstract class AbstractMvPartitionStorageTest extends BaseMvStoragesTest { + /** + * Creates a storage instance for testing. + */ + protected abstract MvPartitionStorage partitionStorage(); + + /** + * Tests that reads and scan from empty storage return empty results. + */ + @Test + public void testEmpty() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + BinaryRow binaryKey = binaryKey(new TestKey(10, "foo")); + + // Read. + assertNull(pk.read(binaryKey, null)); + assertNull(pk.read(binaryKey, Timestamp.nextVersion())); + + // Scan. + assertEquals(List.of(), convert(pk.scan(row -> true, null))); + assertEquals(List.of(), convert(pk.scan(row -> true, Timestamp.nextVersion()))); + } + + /** + * Tests basic invariants of {@link MvPartitionStorage#addWrite(BinaryRow, UUID)}. + */ + @Test + public void testAddWrite() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + TestKey key = new TestKey(10, "foo"); + TestValue value = new TestValue(20, "bar"); + + BinaryRow binaryRow = binaryRow(key, value); + + pk.addWrite(binaryRow, UUID.randomUUID()); + + // Attempt to write from another transaction. + assertThrows(TxIdMismatchException.class, () -> pk.addWrite(binaryRow, UUID.randomUUID())); + + // Read without timestamp returns uncommited row. + assertEquals(value, value(pk.read(binaryKey(key), null))); + + // Read with timestamp returns null. + assertNull(pk.read(binaryKey(key), Timestamp.nextVersion())); + } + + /** + * Tests basic invariants of {@link MvPartitionStorage#abortWrite(BinaryRow)}. + */ + @Test + public void testAbortWrite() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + TestKey key = new TestKey(10, "foo"); + TestValue value = new TestValue(20, "bar"); + + pk.addWrite(binaryRow(key, value), UUID.randomUUID()); + + pk.abortWrite(binaryKey(key)); + + // Aborted row can't be read. + assertNull(pk.read(binaryKey(key), null)); + } + + /** + * Tests basic invariants of {@link MvPartitionStorage#commitWrite(BinaryRow, Timestamp)}. + */ + @Test + public void testCommitWrite() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + TestKey key = new TestKey(10, "foo"); + TestValue value = new TestValue(20, "bar"); + + BinaryRow binaryRow = binaryRow(key, value); + + pk.addWrite(binaryRow, UUID.randomUUID()); + + Timestamp tsBefore = Timestamp.nextVersion(); + + Timestamp tsExact = Timestamp.nextVersion(); + pk.commitWrite(binaryRow, tsExact); + + Timestamp tsAfter = Timestamp.nextVersion(); + + // Row is invisible at the time before writing. + assertNull(pk.read(binaryRow, tsBefore)); + + // Row is valid at the time during and after writing. + assertEquals(value, value(pk.read(binaryRow, null))); + assertEquals(value, value(pk.read(binaryRow, tsExact))); + assertEquals(value, value(pk.read(binaryRow, tsAfter))); + + TestValue newValue = new TestValue(30, "duh"); + + pk.addWrite(binaryRow(key, newValue), UUID.randomUUID()); + + // Same checks, but now there are two different versions. + assertNull(pk.read(binaryRow, tsBefore)); + + assertEquals(newValue, value(pk.read(binaryRow, null))); + + assertEquals(value, value(pk.read(binaryRow, tsExact))); + assertEquals(value, value(pk.read(binaryRow, tsAfter))); + assertEquals(value, value(pk.read(binaryRow, Timestamp.nextVersion()))); + + // Only latest time behavior changes after commit. + pk.commitWrite(binaryKey(key), Timestamp.nextVersion()); + + assertEquals(newValue, value(pk.read(binaryRow, null))); + + assertEquals(value, value(pk.read(binaryRow, tsExact))); + assertEquals(value, value(pk.read(binaryRow, tsAfter))); + + assertEquals(newValue, value(pk.read(binaryRow, Timestamp.nextVersion()))); + + // Remove. + pk.addWrite(binaryKey(key), UUID.randomUUID()); + + assertNull(pk.read(binaryRow, tsBefore)); + + assertNull(pk.read(binaryRow, null)); + + assertEquals(value, value(pk.read(binaryRow, tsExact))); + assertEquals(value, value(pk.read(binaryRow, tsAfter))); + + assertEquals(newValue, value(pk.read(binaryRow, Timestamp.nextVersion()))); + + // Commit remove. + Timestamp removeTs = Timestamp.nextVersion(); + pk.commitWrite(binaryKey(key), removeTs); + + assertNull(pk.read(binaryRow, tsBefore)); + + assertNull(pk.read(binaryRow, null)); + assertNull(pk.read(binaryRow, removeTs)); + assertNull(pk.read(binaryRow, Timestamp.nextVersion())); + + assertEquals(value, value(pk.read(binaryRow, tsExact))); + assertEquals(value, value(pk.read(binaryRow, tsAfter))); + } + + /** + * Tests basic invariants of {@link MvPartitionStorage#scan(Predicate, Timestamp)}. + */ + @Test + public void testScan() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + TestKey key1 = new TestKey(1, "1"); + TestValue value1 = new TestValue(10, "xxx"); + + TestKey key2 = new TestKey(2, "2"); + TestValue value2 = new TestValue(20, "yyy"); + + pk.addWrite(binaryRow(key1, value1), UUID.randomUUID()); + pk.addWrite(binaryRow(key2, value2), UUID.randomUUID()); + + // Scan with and without filters. + assertEquals(List.of(value1, value2), convert(pk.scan(row -> true, null))); + assertEquals(List.of(value1), convert(pk.scan(row -> key(row).intKey == 1, null))); + assertEquals(List.of(value2), convert(pk.scan(row -> key(row).intKey == 2, null))); + + Timestamp ts1 = Timestamp.nextVersion(); + + Timestamp ts2 = Timestamp.nextVersion(); + pk.commitWrite(binaryKey(key1), ts2); + + Timestamp ts3 = Timestamp.nextVersion(); + + Timestamp ts4 = Timestamp.nextVersion(); + pk.commitWrite(binaryKey(key2), ts4); + + Timestamp ts5 = Timestamp.nextVersion(); + + // Full scan with various timestamp values. + assertEquals(List.of(), convert(pk.scan(row -> true, ts1))); + + assertEquals(List.of(value1), convert(pk.scan(row -> true, ts2))); + assertEquals(List.of(value1), convert(pk.scan(row -> true, ts3))); + + assertEquals(List.of(value1, value2), convert(pk.scan(row -> true, ts4))); + assertEquals(List.of(value1, value2), convert(pk.scan(row -> true, ts5))); + } + + private List<TestValue> convert(Cursor<BinaryRow> cursor) throws Exception { + try (cursor) { + return StreamSupport.stream(cursor.spliterator(), false) + .map(BaseMvStoragesTest::value) + .sorted(Comparator.nullsFirst(Comparator.naturalOrder())) + .collect(Collectors.toList()); Review Comment: If we import `Collectors.toList())` statically, the code will read `.collect(toList())` which is almost plain English. At the same time, this will not create an ambiguity because everyone knows that `toList()` returning a `Collector` comes from `Collections`. ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.basic; + +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV partition storage. + */ +public class TestMvPartitionStorage implements MvPartitionStorage { + private final ConcurrentHashMap<ByteBuffer, VersionChain> map = new ConcurrentHashMap<>(); + + private final List<TestSortedIndexMvStorage> indexes; + + public TestMvPartitionStorage(List<TestSortedIndexMvStorage> indexes) { + this.indexes = indexes; + } + + private static class VersionChain { + final @Nullable BinaryRow row; + final @Nullable Timestamp begin; + final @Nullable UUID txId; + final @Nullable VersionChain next; + + VersionChain(@Nullable BinaryRow row, @Nullable Timestamp begin, @Nullable UUID txId, @Nullable VersionChain next) { + this.row = row; + this.begin = begin; + this.txId = txId; + this.next = next; + } + } + + /** {@inheritDoc} */ + @Override + public void addWrite(BinaryRow row, UUID txId) throws TxIdMismatchException { + map.compute(row.keySlice(), (keyBuf, versionChain) -> { + if (versionChain != null && versionChain.begin == null && !txId.equals(versionChain.txId)) { + throw new TxIdMismatchException(); Review Comment: Wouldn't it be useful to have information about both transaction IDs in the exception? ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorageTest.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.basic; + +import java.util.List; +import org.apache.ignite.internal.storage.AbstractMvPartitionStorageTest; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.junit.jupiter.api.BeforeEach; + +/** + * MV partition storage test implementation for {@link TestMvPartitionStorage} class. + */ +public class TestMvPartitionStorageTest extends AbstractMvPartitionStorageTest { + /** Test partition storage instance. */ + private TestMvPartitionStorage storage; + + @BeforeEach + void setUp() { + storage = new TestMvPartitionStorage(List.of()); Review Comment: The initialization of `storage` can be moved to the field initializer, making the field `final` and also making the `@BeforeEach` unnecessary (so the code as a whole simpler). ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java: ########## @@ -0,0 +1,78 @@ +/* + * 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; + +import java.util.UUID; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Multi-versioned partition storage. + * POC version, that represents a combination between a replicated TX-aware MV storage and physical MV storage. Their API are very similar, + * although there are very important differences that will be addressed in the future. + */ +public interface MvPartitionStorage { + /** + * Reads the value from the storage as it was at the given timestamp. {@code null} timestamp means reading the latest value. + * + * @param key Key. + * @param timestamp Timestamp. + * @return Binary row that corresponds to the key or {@code null} if value is not found. + */ + @Nullable + BinaryRow read(BinaryRow key, @Nullable Timestamp timestamp); + + /** + * Creates an uncommitted version, assigned to the given transaction id. + * + * @param row Binary row to update. Key only row means value removal. + * @param txId Transaction id. + * @throws TxIdMismatchException If there's another pending update associated with different transaction id. + * @throws StorageException If failed to write data to the storage. + */ + void addWrite(BinaryRow row, UUID txId) throws TxIdMismatchException, StorageException; Review Comment: It seems a bit scary to fix the decision to use `UUID` to represent transaction IDs. How about introducing a class (or even interface) `TxId`? For now, it could just encapsulate a UUID inside. ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.basic; + +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV partition storage. + */ +public class TestMvPartitionStorage implements MvPartitionStorage { + private final ConcurrentHashMap<ByteBuffer, VersionChain> map = new ConcurrentHashMap<>(); + + private final List<TestSortedIndexMvStorage> indexes; + + public TestMvPartitionStorage(List<TestSortedIndexMvStorage> indexes) { + this.indexes = indexes; Review Comment: Let's make a defensive copy. Performance is not critical here, better safe than sorry. ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.basic; + +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV partition storage. + */ +public class TestMvPartitionStorage implements MvPartitionStorage { + private final ConcurrentHashMap<ByteBuffer, VersionChain> map = new ConcurrentHashMap<>(); + + private final List<TestSortedIndexMvStorage> indexes; + + public TestMvPartitionStorage(List<TestSortedIndexMvStorage> indexes) { + this.indexes = indexes; + } + + private static class VersionChain { Review Comment: This class seems to represent a link of a version chain, not a chain as a whole. Should it be renamed? ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/BaseMvStoragesTest.java: ########## @@ -0,0 +1,179 @@ +/* + * 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; + +import java.util.Locale; +import java.util.Objects; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.schema.Column; +import org.apache.ignite.internal.schema.NativeTypes; +import org.apache.ignite.internal.schema.SchemaDescriptor; +import org.apache.ignite.internal.schema.marshaller.KvMarshaller; +import org.apache.ignite.internal.schema.marshaller.MarshallerException; +import org.apache.ignite.internal.schema.marshaller.MarshallerFactory; +import org.apache.ignite.internal.schema.marshaller.reflection.ReflectionMarshallerFactory; +import org.apache.ignite.internal.schema.row.Row; +import org.apache.ignite.lang.IgniteException; +import org.jetbrains.annotations.Nullable; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; + +/** + * Base test for MV storages, contains pojo classes, their descriptor and a marshaller instance. + */ +public class BaseMvStoragesTest { Review Comment: I suggest making the class abstract so that junit not be confused. ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java: ########## @@ -0,0 +1,233 @@ +/* + * 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; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Comparator; +import java.util.List; +import java.util.UUID; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.junit.jupiter.api.Test; + +/** + * Base test for MV partition storages. + */ +public abstract class AbstractMvPartitionStorageTest extends BaseMvStoragesTest { + /** + * Creates a storage instance for testing. + */ + protected abstract MvPartitionStorage partitionStorage(); + + /** + * Tests that reads and scan from empty storage return empty results. + */ + @Test + public void testEmpty() throws Exception { + MvPartitionStorage pk = partitionStorage(); + + BinaryRow binaryKey = binaryKey(new TestKey(10, "foo")); + + // Read. + assertNull(pk.read(binaryKey, null)); + assertNull(pk.read(binaryKey, Timestamp.nextVersion())); + + // Scan. + assertEquals(List.of(), convert(pk.scan(row -> true, null))); + assertEquals(List.of(), convert(pk.scan(row -> true, Timestamp.nextVersion()))); + } + + /** + * Tests basic invariants of {@link MvPartitionStorage#addWrite(BinaryRow, UUID)}. + */ + @Test + public void testAddWrite() throws Exception { Review Comment: A stylistic nitpick: an ideal test tests just one property of the system. Here, a test tests quite a few properties of one method. Separation to N atomic test methods would make the 'asserted properties' easier to spot, and a failure of a test would make it easier to understand what happens, without resorting to reading code (method name would tell what broke). ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestSortedIndexMvStorage.java: ########## @@ -0,0 +1,249 @@ +/* + * 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.basic; + +import java.util.Comparator; +import java.util.Iterator; +import java.util.Map; +import java.util.NavigableSet; +import java.util.Objects; +import java.util.concurrent.ConcurrentSkipListSet; +import java.util.function.IntPredicate; +import java.util.function.ToIntFunction; +import org.apache.ignite.configuration.NamedListView; +import org.apache.ignite.configuration.schemas.table.ColumnView; +import org.apache.ignite.configuration.schemas.table.IndexColumnView; +import org.apache.ignite.configuration.schemas.table.SortedIndexView; +import org.apache.ignite.configuration.schemas.table.TableIndexView; +import org.apache.ignite.configuration.schemas.table.TableView; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.schema.NativeType; +import org.apache.ignite.internal.schema.SchemaDescriptor; +import org.apache.ignite.internal.schema.configuration.SchemaConfigurationConverter; +import org.apache.ignite.internal.schema.configuration.SchemaDescriptorConverter; +import org.apache.ignite.internal.schema.row.Row; +import org.apache.ignite.internal.storage.index.IndexRowPrefix; +import org.apache.ignite.internal.storage.index.PrefixComparator; +import org.apache.ignite.internal.storage.index.SortedIndexMvStorage; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV sorted index storage. + */ +public class TestSortedIndexMvStorage implements SortedIndexMvStorage { + private final NavigableSet<BinaryRow> index; + + private final SchemaDescriptor descriptor; + + private final Map<Integer, TestMvPartitionStorage> pk; + + private final int partitions; + + private final IndexColumnView[] indexColumns; + + private final int[] columnIndexes; + + private final NativeType[] nativeTypes; + + /** + * Constructor. + */ + public TestSortedIndexMvStorage( + String name, + TableView tableCfg, + SchemaDescriptor descriptor, + Map<Integer, TestMvPartitionStorage> pk + ) { + this.descriptor = descriptor; + + this.pk = pk; Review Comment: A defensive copy? ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/BaseMvStoragesTest.java: ########## @@ -0,0 +1,179 @@ +/* + * 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; + +import java.util.Locale; +import java.util.Objects; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.schema.Column; +import org.apache.ignite.internal.schema.NativeTypes; +import org.apache.ignite.internal.schema.SchemaDescriptor; +import org.apache.ignite.internal.schema.marshaller.KvMarshaller; +import org.apache.ignite.internal.schema.marshaller.MarshallerException; +import org.apache.ignite.internal.schema.marshaller.MarshallerFactory; +import org.apache.ignite.internal.schema.marshaller.reflection.ReflectionMarshallerFactory; +import org.apache.ignite.internal.schema.row.Row; +import org.apache.ignite.lang.IgniteException; +import org.jetbrains.annotations.Nullable; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; + +/** + * Base test for MV storages, contains pojo classes, their descriptor and a marshaller instance. + */ +public class BaseMvStoragesTest { + /** Default reflection marshaller factory. */ + protected static MarshallerFactory marshallerFactory; Review Comment: Why are these fields static? The objects put into them do not seem to be heavy to be created, so they can be recreated for each test method invocation. This would tests isolation better (less probability that some of these objects would store some state interfering some test cases). Also, it looks like the fields could be initialized via field initializers (instead of `@BeforeEach`/`@BeforeAll`), and the fields could be made `final` ( -> less worries, especially given that this is a base class of a test class hierarchy). ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestSortedIndexMvStorage.java: ########## @@ -0,0 +1,249 @@ +/* + * 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.basic; + +import java.util.Comparator; +import java.util.Iterator; +import java.util.Map; +import java.util.NavigableSet; +import java.util.Objects; +import java.util.concurrent.ConcurrentSkipListSet; +import java.util.function.IntPredicate; +import java.util.function.ToIntFunction; +import org.apache.ignite.configuration.NamedListView; +import org.apache.ignite.configuration.schemas.table.ColumnView; +import org.apache.ignite.configuration.schemas.table.IndexColumnView; +import org.apache.ignite.configuration.schemas.table.SortedIndexView; +import org.apache.ignite.configuration.schemas.table.TableIndexView; +import org.apache.ignite.configuration.schemas.table.TableView; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.schema.NativeType; +import org.apache.ignite.internal.schema.SchemaDescriptor; +import org.apache.ignite.internal.schema.configuration.SchemaConfigurationConverter; +import org.apache.ignite.internal.schema.configuration.SchemaDescriptorConverter; +import org.apache.ignite.internal.schema.row.Row; +import org.apache.ignite.internal.storage.index.IndexRowPrefix; +import org.apache.ignite.internal.storage.index.PrefixComparator; +import org.apache.ignite.internal.storage.index.SortedIndexMvStorage; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV sorted index storage. + */ +public class TestSortedIndexMvStorage implements SortedIndexMvStorage { + private final NavigableSet<BinaryRow> index; + + private final SchemaDescriptor descriptor; + + private final Map<Integer, TestMvPartitionStorage> pk; + + private final int partitions; + + private final IndexColumnView[] indexColumns; + + private final int[] columnIndexes; + + private final NativeType[] nativeTypes; + + /** + * Constructor. + */ + public TestSortedIndexMvStorage( + String name, + TableView tableCfg, + SchemaDescriptor descriptor, + Map<Integer, TestMvPartitionStorage> pk + ) { + this.descriptor = descriptor; + + this.pk = pk; + + partitions = tableCfg.partitions(); + + index = new ConcurrentSkipListSet<>(((Comparator<BinaryRow>) this::compareColumns).thenComparing(BinaryRow::keySlice)); + + // Init columns. + NamedListView<? extends ColumnView> tblColumns = tableCfg.columns(); + + TableIndexView idxCfg = tableCfg.indices().get(name); + + assert idxCfg instanceof SortedIndexView; + + SortedIndexView sortedIdxCfg = (SortedIndexView) idxCfg; + + NamedListView<? extends IndexColumnView> columns = sortedIdxCfg.columns(); + + int length = columns.size(); + + this.indexColumns = new IndexColumnView[length]; + this.columnIndexes = new int[length]; + this.nativeTypes = new NativeType[length]; + + for (int i = 0; i < length; i++) { + IndexColumnView idxColumn = columns.get(i); + + indexColumns[i] = idxColumn; + + int columnIndex = tblColumns.namedListKeys().indexOf(idxColumn.name()); + + columnIndexes[i] = columnIndex; + + nativeTypes[i] = SchemaDescriptorConverter.convert(SchemaConfigurationConverter.convert(tblColumns.get(columnIndex).type())); + } + } + + /** {@inheritDoc} */ + @Override + public boolean supportsBackwardsScan() { + return true; + } + + /** {@inheritDoc} */ + @Override + public boolean supportsIndexOnlyScan() { + return false; + } + + private int compareColumns(BinaryRow l, BinaryRow r) { + Row leftRow = new Row(descriptor, l); + Row rightRow = new Row(descriptor, r); + + for (int i = 0; i < indexColumns.length; i++) { + int columnIndex = columnIndexes[i]; + + int cmp = PrefixComparator.compareColumns(leftRow, columnIndex, nativeTypes[i].spec(), rightRow.value(columnIndex)); + + if (cmp != 0) { + return indexColumns[i].asc() ? cmp : -cmp; + } + } + + return 0; + } + + public void append(BinaryRow row) { + index.add(row); + } + + public void remove(BinaryRow row) { + index.remove(row); + } + + public boolean matches(BinaryRow aborted, BinaryRow existing) { Review Comment: Why are the parameters called the way they are? Is it only used in the 'write abort' scenario? It just seems weird that the naming of the parameters is so highly specialized, while the name of the method is pretty generic. ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.basic; + +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV partition storage. + */ +public class TestMvPartitionStorage implements MvPartitionStorage { + private final ConcurrentHashMap<ByteBuffer, VersionChain> map = new ConcurrentHashMap<>(); Review Comment: Is it required to have the concrete `ConcurrentHashMap` as the field type? Isn't `ConcurrentMap` possible? ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.basic; + +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.TxIdMismatchException; +import org.apache.ignite.internal.tx.Timestamp; +import org.apache.ignite.internal.util.Cursor; +import org.jetbrains.annotations.Nullable; + +/** + * Test implementation of MV partition storage. + */ +public class TestMvPartitionStorage implements MvPartitionStorage { + private final ConcurrentHashMap<ByteBuffer, VersionChain> map = new ConcurrentHashMap<>(); + + private final List<TestSortedIndexMvStorage> indexes; + + public TestMvPartitionStorage(List<TestSortedIndexMvStorage> indexes) { + this.indexes = indexes; + } + + private static class VersionChain { + final @Nullable BinaryRow row; + final @Nullable Timestamp begin; + final @Nullable UUID txId; + final @Nullable VersionChain next; + + VersionChain(@Nullable BinaryRow row, @Nullable Timestamp begin, @Nullable UUID txId, @Nullable VersionChain next) { + this.row = row; + this.begin = begin; + this.txId = txId; + this.next = next; + } + } + + /** {@inheritDoc} */ + @Override + public void addWrite(BinaryRow row, UUID txId) throws TxIdMismatchException { + map.compute(row.keySlice(), (keyBuf, versionChain) -> { + if (versionChain != null && versionChain.begin == null && !txId.equals(versionChain.txId)) { + throw new TxIdMismatchException(); + } + + return new VersionChain(row, null, txId, versionChain); Review Comment: We could add a static method (to `VersionChain`) like `newUncommitted(BinaryRow, UUID, VersionChain)`, so the code would look like `return newUncommitted(row, txId, versionChain)`, which would eliminate confusion about 'what does `null` stand for here'? ########## modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestSortedIndexMvStorageTest.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.storage.basic; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; +import org.apache.ignite.configuration.schemas.table.TableView; +import org.apache.ignite.internal.storage.AbstractSortedIndexMvStorageTest; +import org.apache.ignite.internal.storage.MvPartitionStorage; +import org.apache.ignite.internal.storage.index.SortedIndexMvStorage; +import org.junit.jupiter.api.BeforeEach; + +/** + * MV sorted index storage test implementation for {@link TestSortedIndexMvStorage} class. + */ +public class TestSortedIndexMvStorageTest extends AbstractSortedIndexMvStorageTest { + + private List<TestSortedIndexMvStorage> indexes; + + private TestMvPartitionStorage partitionStorage; + + @BeforeEach + void setUp() { + indexes = new CopyOnWriteArrayList<>(); Review Comment: Initialization of these fields can be moved to field initiailzers -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
