sashapolo commented on code in PR #1213:
URL: https://github.com/apache/ignite-3/pull/1213#discussion_r997021755
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/hash/io/HashIndexTreeIo.java:
##########
@@ -262,10 +263,9 @@ default HashIndexRow getRow(DataPageReader dataPageReader,
int partitionId, long
indexColumnsBuffer =
ByteBuffer.wrap(indexColumnsTraversal.result());
}
- IndexColumns indexColumns = new IndexColumns(partitionId, link,
indexColumnsBuffer);
+ IndexColumns indexColumns = new IndexColumns(partitionId, link,
indexColumnsBuffer.order(LITTLE_ENDIAN));
Review Comment:
I think it would be a little bit more beautiful, if you made
`indexColumnsBuffer` a `byte[]` and wrapped it into a bytebuffer here
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/SortedIndexTree.java:
##########
@@ -67,18 +74,29 @@ public SortedIndexTree(
AtomicLong globalRmvId,
long metaPageId,
@Nullable ReuseList reuseList,
- boolean initNew,
- BinaryTupleComparator binaryTupleComparator
+ SortedIndexDescriptor indexDescriptor,
+ boolean initNew
) throws IgniteInternalCheckedException {
super("SortedIndexTree_" + grpId, grpId, grpName, partId, pageMem,
lockLsnr, globalRmvId, metaPageId, reuseList);
- setIos(SortedIndexTreeInnerIo.VERSIONS,
SortedIndexTreeLeafIo.VERSIONS, SortedIndexTreeMetaIo.VERSIONS);
+ inlineSize = initNew ? InlineUtils.binaryTupleInlineSize(pageSize(),
ITEM_SIZE_WITHOUT_COLUMNS, indexDescriptor)
+ : readInlineSizeFromMetaIo();
+
+ setIos(
+ SortedIndexTreeInnerIo.VERSIONS.get(inlineSize),
+ SortedIndexTreeLeafIo.VERSIONS.get(inlineSize),
+ SortedIndexTreeMetaIo.VERSIONS
+ );
dataPageReader = new DataPageReader(pageMem, grpId,
statisticsHolder());
- this.binaryTupleComparator = binaryTupleComparator;
+ binaryTupleComparator = new BinaryTupleComparator(indexDescriptor);
initTree(initNew);
+
+ if (initNew) {
+ writeInlineSizeToMetaIo(inlineSize);
Review Comment:
what if we fail for some reason before we had the chance to write the size
to Meta IO?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -76,25 +113,37 @@ default void store(long dstPageAddr, int dstIdx,
BplusIo<SortedIndexRowKey> srcI
int dstOffset = offset(dstIdx);
int srcOffset = offset(srcIdx);
- PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
SIZE_IN_BYTES);
+ PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
getItemSize());
}
/**
* Stores a sorted index row in the page.
*
* @see BplusIo#storeByOffset(long, int, Object)
*/
- default void storeByOffset(long pageAddr, int off, SortedIndexRowKey
rowKey) {
+ default void storeByOffset(long pageAddr, final int off, SortedIndexRowKey
rowKey) {
Review Comment:
what's `final` here for?
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/index/IndexTestUtils.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.pagememory.index;
+
+import static java.util.stream.Collectors.joining;
+
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.stream.IntStream;
+
+/**
+ * Helper class for testing indexes.
+ */
+class IndexTestUtils {
+ /**
+ * Returns a string with pseudo-random characters from 'A' to 'Z'.
+ *
+ * @param size Number of characters.
+ */
+ static String randomString(int size) {
Review Comment:
There already exists a similar method in
`org.apache.ignite.internal.testframework.IgniteTestUtils`, so I think this
class should be removed
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -76,25 +113,37 @@ default void store(long dstPageAddr, int dstIdx,
BplusIo<SortedIndexRowKey> srcI
int dstOffset = offset(dstIdx);
int srcOffset = offset(srcIdx);
- PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
SIZE_IN_BYTES);
+ PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
getItemSize());
}
/**
* Stores a sorted index row in the page.
*
* @see BplusIo#storeByOffset(long, int, Object)
*/
- default void storeByOffset(long pageAddr, int off, SortedIndexRowKey
rowKey) {
+ default void storeByOffset(long pageAddr, final int off, SortedIndexRowKey
rowKey) {
assert rowKey instanceof SortedIndexRow;
- SortedIndexRow sortedIndexRow = (SortedIndexRow) rowKey;
+ SortedIndexRow row = (SortedIndexRow) rowKey;
+
+ IndexColumns indexColumns = row.indexColumns();
- writePartitionless(pageAddr + off + INDEX_COLUMNS_LINK_OFFSET,
sortedIndexRow.indexColumns().link());
+ if (isFullyInlined(indexColumns.valueSize(),
indexColumnsInlineSize())) {
Review Comment:
I think this method should be called `canFullyInline`
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/SortedIndexTree.java:
##########
@@ -43,6 +47,9 @@ public class SortedIndexTree extends
BplusTree<SortedIndexRowKey, SortedIndexRow
/** Comparator of index columns {@link BinaryTuple}s. */
private final BinaryTupleComparator binaryTupleComparator;
+ /** Inline size in bytes. */
+ private final int inlineSize;
Review Comment:
why do you need this field?
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/index/AbstractPageMemoryHashIndexStorageTest.java:
##########
@@ -63,18 +61,6 @@ protected final void initialize(
public void testDestroy() {
}
- @Test
- void testWithHashCollisionStrings() {
Review Comment:
Why did you remove this test?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -48,17 +55,47 @@
* </ul>
*/
public interface SortedIndexTreeIo {
- /** Offset of the index column link (6 bytes). */
- int INDEX_COLUMNS_LINK_OFFSET = 0;
+ /** Item size without index columns in bytes. */
+ int ITEM_SIZE_WITHOUT_COLUMNS = Short.SIZE // Inlined index columns size.
+ + PARTITIONLESS_LINK_SIZE_BYTES // Index columns link.
+ + 2 * Long.BYTES; // Row ID.
- /** Offset of rowId's most significant bits, 8 bytes. */
- int ROW_ID_MSB_OFFSET = INDEX_COLUMNS_LINK_OFFSET +
PARTITIONLESS_LINK_SIZE_BYTES;
+ /** Index columns are not fully inlined, the index column size is {@link
#indexColumnsInlineSize()}. */
Review Comment:
I don't understand, how is is constant expected to be used? Maybe it should
be called `NOT_FULLY_INLINED_FLAG`? And why is it a `short`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -76,25 +113,37 @@ default void store(long dstPageAddr, int dstIdx,
BplusIo<SortedIndexRowKey> srcI
int dstOffset = offset(dstIdx);
int srcOffset = offset(srcIdx);
- PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
SIZE_IN_BYTES);
+ PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
getItemSize());
}
/**
* Stores a sorted index row in the page.
*
* @see BplusIo#storeByOffset(long, int, Object)
*/
- default void storeByOffset(long pageAddr, int off, SortedIndexRowKey
rowKey) {
+ default void storeByOffset(long pageAddr, final int off, SortedIndexRowKey
rowKey) {
assert rowKey instanceof SortedIndexRow;
- SortedIndexRow sortedIndexRow = (SortedIndexRow) rowKey;
+ SortedIndexRow row = (SortedIndexRow) rowKey;
+
+ IndexColumns indexColumns = row.indexColumns();
- writePartitionless(pageAddr + off + INDEX_COLUMNS_LINK_OFFSET,
sortedIndexRow.indexColumns().link());
+ if (isFullyInlined(indexColumns.valueSize(),
indexColumnsInlineSize())) {
+ putShort(pageAddr + off, SIZE_OFFSET, (short)
indexColumns.valueSize());
- RowId rowId = sortedIndexRow.rowId();
+ putByteBuffer(pageAddr + off, TUPLE_OFFSET,
indexColumns.valueBuffer().rewind());
Review Comment:
why do you need to call `rewind` here?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -117,39 +166,48 @@ default int compare(
int idx,
SortedIndexRowKey rowKey
) throws IgniteInternalCheckedException {
- int off = offset(idx);
+ final int off = offset(idx);
Review Comment:
why `final`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -164,23 +222,42 @@ default int compare(
*/
default SortedIndexRow getRow(DataPageReader dataPageReader, int
partitionId, long pageAddr, int idx)
throws IgniteInternalCheckedException {
- int off = offset(idx);
+ final int off = offset(idx);
Review Comment:
```suggestion
int off = offset(idx);
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/io/SortedIndexTreeIo.java:
##########
@@ -76,25 +113,37 @@ default void store(long dstPageAddr, int dstIdx,
BplusIo<SortedIndexRowKey> srcI
int dstOffset = offset(dstIdx);
int srcOffset = offset(srcIdx);
- PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
SIZE_IN_BYTES);
+ PageUtils.copyMemory(srcPageAddr, srcOffset, dstPageAddr, dstOffset,
getItemSize());
}
/**
* Stores a sorted index row in the page.
*
* @see BplusIo#storeByOffset(long, int, Object)
*/
- default void storeByOffset(long pageAddr, int off, SortedIndexRowKey
rowKey) {
+ default void storeByOffset(long pageAddr, final int off, SortedIndexRowKey
rowKey) {
assert rowKey instanceof SortedIndexRow;
- SortedIndexRow sortedIndexRow = (SortedIndexRow) rowKey;
+ SortedIndexRow row = (SortedIndexRow) rowKey;
+
+ IndexColumns indexColumns = row.indexColumns();
- writePartitionless(pageAddr + off + INDEX_COLUMNS_LINK_OFFSET,
sortedIndexRow.indexColumns().link());
+ if (isFullyInlined(indexColumns.valueSize(),
indexColumnsInlineSize())) {
+ putShort(pageAddr + off, SIZE_OFFSET, (short)
indexColumns.valueSize());
- RowId rowId = sortedIndexRow.rowId();
+ putByteBuffer(pageAddr + off, TUPLE_OFFSET,
indexColumns.valueBuffer().rewind());
+ } else {
+ putShort(pageAddr + off, SIZE_OFFSET, NOT_FULLY_INLINE);
- putLong(pageAddr, off + ROW_ID_MSB_OFFSET,
rowId.mostSignificantBits());
- putLong(pageAddr, off + ROW_ID_LSB_OFFSET,
rowId.leastSignificantBits());
+ putByteBuffer(pageAddr + off, TUPLE_OFFSET,
indexColumns.valueBuffer().rewind().duplicate().limit(indexColumnsInlineSize()));
Review Comment:
Please extract these buffer-related stuff into a variable
--
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]
