[GitHub] [ignite-3] ibessonov commented on a change in pull request #468: IGNITE-15885 Implement RocksDB-based Sorted Index Storage

[GitBox <gi...@apache.org>]

ibessonov commented on a change in pull request #468:
URL: https://github.com/apache/ignite-3/pull/468#discussion_r766688539



##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/ByteBufferRow.java
##########
@@ -137,7 +137,11 @@ public double readDouble(int off) {
     /** {@inheritDoc} */
     @Override
     public String readString(int off, int len) {
-        return new String(buf.array(), off, len, StandardCharsets.UTF_8);
+        if (buf.hasArray()) {
+            return new String(buf.array(), off, len, StandardCharsets.UTF_8);

Review comment:
       Just curious, will it work for sliced byte buffer?

##########
File path: modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexDescriptor.java
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toUnmodifiableList;
+
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Stream;
+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.Column;
+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.storage.StorageException;
+import org.apache.ignite.schema.definition.ColumnDefinition;
+
+/**
+ * Descriptor for creating a Sorted Index Storage.
+ *
+ * @see SortedIndexStorage
+ */
+public class SortedIndexDescriptor {
+    /**
+     * Descriptor of a Sorted Index column (column name and column sort order).
+     */
+    public static class ColumnDescriptor {
+        private final Column column;
+
+        private final boolean asc;
+
+        private final boolean indexedColumn;
+
+        ColumnDescriptor(Column column, boolean asc, boolean indexedColumn) {
+            this.column = column;
+            this.asc = asc;
+            this.indexedColumn = indexedColumn;
+        }
+
+        /**
+         * Returns a column descriptor.
+         */
+        public Column column() {
+            return column;
+        }
+
+        /**
+         * Returns {@code true} if this column is sorted in ascending order or {@code false} otherwise.
+         */
+        public boolean asc() {
+            return asc;
+        }
+
+        /**
+         * Returns {@code true} if this column was explicitly marked as an indexed column or {@code false} if it is a part of a Primary Key
+         * appended for uniqueness.
+         */
+        public boolean indexedColumn() {
+            return indexedColumn;
+        }
+
+        @Override
+        public String toString() {
+            return "ColumnDescriptor{"
+                    + "name=" + column.name()
+                    + ", type=" + column.type().spec()
+                    + ", asc=" + asc
+                    + ", indexedColumn=" + indexedColumn
+                    + '}';
+        }
+    }
+
+    private final String name;
+
+    private final List<ColumnDescriptor> columns;
+
+    private final SchemaDescriptor schemaDescriptor;
+
+    /**
+     * Creates an Index Descriptor from a given Table Configuration.
+     *
+     * @param name        index name.
+     * @param tableConfig table configuration.
+     */
+    public SortedIndexDescriptor(String name, TableView tableConfig) {
+        this.name = name;
+
+        TableIndexView indexConfig = tableConfig.indices().get(name);
+
+        if (indexConfig == null) {
+            throw new StorageException(String.format("Index configuration for \"%s\" could not be found", name));
+        }
+
+        if (!(indexConfig instanceof SortedIndexView)) {
+            throw new StorageException(String.format(
+                    "Index \"%s\" is not configured as a Sorted Index. Actual type: %s",
+                    name, indexConfig.type()
+            ));
+        }
+
+        // extract indexed column configurations from the table configuration
+        NamedListView<? extends IndexColumnView> indexColumns = ((SortedIndexView) indexConfig).columns();
+
+        Stream<String> indexColumnNames = indexColumns.namedListKeys().stream()

Review comment:
       I'm a little bit confused, but maybe what you need here is indexColumns.namedListKeys().stream(). Maybe I'm wrong, please check it.

##########
File path: modules/storage-rocksdb/src/test/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorageTest.java
##########
@@ -0,0 +1,468 @@
+/*
+ * 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.rocksdb.index;
+
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toUnmodifiableList;
+import static org.apache.ignite.internal.schema.SchemaTestUtils.generateRandomValue;
+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.schema.SchemaBuilders.column;
+import static org.apache.ignite.schema.SchemaBuilders.tableBuilder;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.anyOf;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.sameInstance;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Predicate;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+import org.apache.ignite.configuration.schemas.store.DataRegionConfiguration;
+import org.apache.ignite.configuration.schemas.table.HashIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.PartialIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.SortedIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfiguration;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.storage.SearchRow;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.internal.storage.engine.DataRegion;
+import org.apache.ignite.internal.storage.engine.TableStorage;
+import org.apache.ignite.internal.storage.index.IndexRow;
+import org.apache.ignite.internal.storage.index.IndexRowPrefix;
+import org.apache.ignite.internal.storage.index.SortedIndexDescriptor.ColumnDescriptor;
+import org.apache.ignite.internal.storage.index.SortedIndexStorage;
+import org.apache.ignite.internal.storage.rocksdb.RocksDbStorageEngine;
+import org.apache.ignite.internal.testframework.VariableSource;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.apache.ignite.lang.IgniteLogger;
+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.builder.SortedIndexDefinitionBuilder;
+import org.apache.ignite.schema.definition.builder.SortedIndexDefinitionBuilder.SortedIndexColumnBuilder;
+import org.apache.ignite.schema.definition.index.ColumnarIndexDefinition;
+import org.apache.ignite.schema.definition.index.HashIndexDefinition;
+import org.apache.ignite.schema.definition.index.SortedIndexDefinition;
+import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+
+/**
+ * Test class for the {@link RocksDbSortedIndexStorage}.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+@ExtendWith(ConfigurationExtension.class)
+public class RocksDbSortedIndexStorageTest {
+    private static final IgniteLogger log = IgniteLogger.forClass(RocksDbSortedIndexStorageTest.class);
+
+    private static final Random random = new Random();
+
+    /**
+     * Definitions of all supported column types.
+     */
+    private static final List<ColumnDefinition> ALL_TYPES_COLUMN_DEFINITIONS = allTypesColumnDefinitions();
+
+    @InjectConfiguration(polymorphicExtensions = {
+            HashIndexConfigurationSchema.class,

Review comment:
       Having only "SortedIndexConfigurationSchema" is enough here, you can remove the others

##########
File path: modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexDescriptor.java
##########
@@ -0,0 +1,202 @@
+/*
+ * 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 java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toUnmodifiableList;
+
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Stream;
+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.Column;
+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.storage.StorageException;
+import org.apache.ignite.schema.definition.ColumnDefinition;
+
+/**
+ * Descriptor for creating a Sorted Index Storage.
+ *
+ * @see SortedIndexStorage
+ */
+public class SortedIndexDescriptor {
+    /**
+     * Descriptor of a Sorted Index column (column name and column sort order).
+     */
+    public static class ColumnDescriptor {
+        private final Column column;
+
+        private final boolean asc;
+
+        private final boolean indexedColumn;
+
+        ColumnDescriptor(Column column, boolean asc, boolean indexedColumn) {
+            this.column = column;
+            this.asc = asc;
+            this.indexedColumn = indexedColumn;
+        }
+
+        /**
+         * Returns a column descriptor.
+         */
+        public Column column() {
+            return column;
+        }
+
+        /**
+         * Returns {@code true} if this column is sorted in ascending order or {@code false} otherwise.
+         */
+        public boolean asc() {
+            return asc;
+        }
+
+        /**
+         * Returns {@code true} if this column was explicitly marked as an indexed column or {@code false} if it is a part of a Primary Key
+         * appended for uniqueness.
+         */
+        public boolean indexedColumn() {
+            return indexedColumn;
+        }
+
+        @Override
+        public String toString() {
+            return "ColumnDescriptor{"

Review comment:
       Maybe you should use one of S.toString methods

##########
File path: modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.rocksdb.index;
+
+import org.apache.ignite.internal.rocksdb.ColumnFamily;
+import org.apache.ignite.internal.rocksdb.RocksIteratorAdapter;
+import org.apache.ignite.internal.schema.ByteBufferRow;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.internal.storage.index.IndexRow;
+import org.apache.ignite.internal.storage.index.IndexRowDeserializer;
+import org.apache.ignite.internal.storage.index.IndexRowFactory;
+import org.apache.ignite.internal.storage.index.IndexRowPrefix;
+import org.apache.ignite.internal.storage.index.SortedIndexDescriptor;
+import org.apache.ignite.internal.storage.index.SortedIndexStorage;
+import org.apache.ignite.internal.util.Cursor;
+import org.jetbrains.annotations.Nullable;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.RocksIterator;
+
+/**
+ * {@link SortedIndexStorage} implementation based on RocksDB.
+ */
+public class RocksDbSortedIndexStorage implements SortedIndexStorage {
+    private final ColumnFamily indexCf;
+
+    private final SortedIndexDescriptor descriptor;
+
+    private final IndexRowFactory indexRowFactory;
+
+    private final IndexRowDeserializer indexRowDeserializer;
+
+    /**
+     * Creates a new Index storage.
+     *
+     * @param indexCf Column Family for storing the data.
+     * @param descriptor Index descriptor.
+     */
+    public RocksDbSortedIndexStorage(ColumnFamily indexCf, SortedIndexDescriptor descriptor) {
+        this.indexCf = indexCf;
+        this.descriptor = descriptor;
+        this.indexRowFactory = new BinaryIndexRowFactory(descriptor);
+        this.indexRowDeserializer = new BinaryIndexRowDeserializer(descriptor);
+    }
+
+    @Override
+    public SortedIndexDescriptor indexDescriptor() {
+        return descriptor;
+    }
+
+    @Override
+    public IndexRowFactory indexKeyFactory() {
+        return indexRowFactory;
+    }
+
+    @Override
+    public IndexRowDeserializer indexRowDeserializer() {
+        return indexRowDeserializer;
+    }
+
+    @Override
+    public void put(IndexRow row) {
+        try {
+            indexCf.put(row.rowBytes(), row.primaryKey().keyBytes());
+        } catch (RocksDBException e) {
+            throw new StorageException("Error while adding data to Rocks DB", e);
+        }
+    }
+
+    @Override
+    public void remove(IndexRow key) {
+        try {
+            indexCf.delete(key.rowBytes());
+        } catch (RocksDBException e) {
+            throw new StorageException("Error while removing data from Rocks DB", e);
+        }
+    }
+
+    @Override
+    public Cursor<IndexRow> range(IndexRowPrefix lowerBound, IndexRowPrefix upperBound) {
+        RocksIterator iter = indexCf.newIterator();
+
+        iter.seekToFirst();

Review comment:
       Can you call something like ".seek(lowerBound.bytes())" instead of starting from the beginning of the index? Would be more efficient if it's possible




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