rpuch commented on code in PR #6316: URL: https://github.com/apache/ignite-3/pull/6316#discussion_r2238891235
########## modules/core/src/test/java/org/apache/ignite/internal/util/GridUnsafeTest.java: ########## @@ -0,0 +1,47 @@ +/* + * 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.util; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; + +import java.nio.ByteBuffer; +import org.junit.jupiter.api.Test; + +/** Tests for {@link GridUnsafe}. */ +class GridUnsafeTest { + @Test + void getBytes() { + // We must use manual memory allocation, because an instance created by "allocateDirected" might be garbage-collected in the middle Review Comment: ```suggestion // We must use manual memory allocation, because an instance created by "allocateDirect" might be garbage-collected in the middle ``` ########## modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/comparator/JitComparator.java: ########## @@ -20,8 +20,13 @@ import org.apache.ignite.internal.schema.UnsafeByteBufferAccessor; /** - * Interface for comparing two binary tuples, represented as {@link UnsafeByteBufferAccessor}s. All implementation of this interface are + * Interface for comparing two binary tuples, represented as {@link UnsafeByteBufferAccessor}s. All implementations of this interface are * expected to be generated in runtime by {@link JitComparatorGenerator}. + * + * <p>Throughout the codebase I refer to its parameters as {@code outer} and {@code inner}. The meaning behind these terms is the following: + * outer entry might come outside of an index tree, and this it can be a prefix. But inner entry always comes from the index itself. It Review Comment: ```suggestion * outer entry might come outside of an index tree, and thus it can be a prefix. But inner entry always comes from the index itself. It ``` ########## modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/comparator/JitComparatorGenerator.java: ########## @@ -210,9 +212,9 @@ public static JitComparator createComparator( Variable innerEntrySize = scope.declareVariable(int.class, "innerEntrySize"); if (maxEntrySizeLog != 0) { - // If entry size can be more than 1, then we read it from headers of tuples like this: - // int outerFlag = outerAccessor.get(0) & BinaryTupleCommon.VARSIZE_MASK; - // int outerEntrySize = 1 << outerFlag; + // If entry size can be more than 1 byte, then we read it from headers of tuples like this: + // int outerEntrySize = 1 << (outerAccessor.get(0) & BinaryTupleCommon.VARSIZE_MASK); Review Comment: I still only see the shift in the comment, but not in the code. If the comment about 'logarithmic scale' is here to address this, then this doesn't seem to be enough, it's still highly confusing. How about making the comment mirror the actual code? ########## modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/comparator/JitComparator.java: ########## @@ -20,8 +20,13 @@ import org.apache.ignite.internal.schema.UnsafeByteBufferAccessor; /** - * Interface for comparing two binary tuples, represented as {@link UnsafeByteBufferAccessor}s. All implementation of this interface are + * Interface for comparing two binary tuples, represented as {@link UnsafeByteBufferAccessor}s. All implementations of this interface are * expected to be generated in runtime by {@link JitComparatorGenerator}. + * + * <p>Throughout the codebase I refer to its parameters as {@code outer} and {@code inner}. The meaning behind these terms is the following: + * outer entry might come outside of an index tree, and this it can be a prefix. But inner entry always comes from the index itself. It + * cannot be a prefix, but can in principle be a suffix of a full tuple. The Review Comment: How can it be a suffix? Is this important for the usage of this interface, or it's just a 'fun fact'? If it's the latter, should this be mentioned? ########## modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/comparator/JitComparatorGenerator.java: ########## @@ -259,12 +261,18 @@ public static JitComparator createComparator( // "case 2" can be missing if "maxEntrySizeLog" is equal to "1". SwitchBuilder outerSwitchBuilder = switchBuilder() .expression(outerEntrySize) - .defaultCase(constantInt(0).ret()); + .defaultCase(new BytecodeBlock().append(newInstance( + BinaryTupleFormatException.class, + constantString("Invalid header, header size 8 is not supported.") Review Comment: Should it be 8? How about just 'should never be reachable'? -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
