ascherbakoff commented on code in PR #787: URL: https://github.com/apache/ignite-3/pull/787#discussion_r858490759
########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/IgniteRowId.java: ########## @@ -0,0 +1,30 @@ +/* + * 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; + +/** + * Interface that represents row id in primary index of the table. + * + * @see MvPartitionStorage + */ +public interface IgniteRowId extends Comparable<IgniteRowId> { + /** + * Returns a partition id for current row id. + */ + int partitionId(); Review Comment: @return is still missing ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java: ########## @@ -29,43 +29,56 @@ * 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 { +public interface MvPartitionStorage extends AutoCloseable { /** * 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 rowId Row id. * @param timestamp Timestamp. Review Comment: Specify the meaning of timestamp nullability ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/UuidIgniteRowId.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.UUID; +import org.jetbrains.annotations.NotNull; + +/** + * UUID-based ignite row id implementation. + */ +public final class UuidIgniteRowId implements IgniteRowId { + /* + * The most significant 64 bits. + */ + private final long mostSigBits; + + /* + * The least significant 64 bits. + */ + private final long leastSigBits; + + /** + * Constructor. + * + * @param uuid UUID. + */ + public UuidIgniteRowId(UUID uuid) { Review Comment: I think we need to pass a partition here. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/UuidIgniteRowId.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.UUID; +import org.jetbrains.annotations.NotNull; + +/** + * UUID-based ignite row id implementation. + */ +public final class UuidIgniteRowId implements IgniteRowId { + /* + * The most significant 64 bits. + */ + private final long mostSigBits; + + /* + * The least significant 64 bits. + */ + private final long leastSigBits; + + /** + * Constructor. + * + * @param uuid UUID. + */ + public UuidIgniteRowId(UUID uuid) { + this(uuid.getMostSignificantBits(), uuid.getLeastSignificantBits()); + } + + /** Private constructor. */ + private UuidIgniteRowId(long mostSigBits, long leastSigBits) { + this.mostSigBits = mostSigBits; + this.leastSigBits = leastSigBits; + } + + /** + * Returns {@link UuidIgniteRowId} instance based on {@link UUID#randomUUID()}. Highest two bytes of the most significant bits part will + * encode partition id. + * + * @param partitionId Partition id. + */ + public static IgniteRowId randomRowId(int partitionId) { + UUID randomUuid = UUID.randomUUID(); + + long lsb = randomUuid.getLeastSignificantBits(); + + lsb = (lsb & ~0xFFFFL) | partitionId; + + return new UuidIgniteRowId(randomUuid.getMostSignificantBits(), lsb); + } + + /** {@inheritDoc} */ + @Override + public int partitionId() { + return (int) (leastSigBits & 0xFFFFL); + } + + /** + * Writes row id into a byte buffer. Binary row representation should match natural order defined by {@link #compareTo(Object)} when + * comparing lexicographically. + * + * @param buf Output byte buffer with {@link java.nio.ByteOrder#BIG_ENDIAN} byte order. + * @param signedBytesCompare Defines properties of a target binary comparator. {@code true} if bytes are compared as signed values, + * {@code false} if unsigned. + */ + public void writeTo(ByteBuffer buf, boolean signedBytesCompare) { Review Comment: Why do we need two compare variants? It makes no sense to me, because the order on rowIds is irrelevant. So any order will suffice for storing in a binary sorted tree like rocksdb. We can just use the default lexicographic order of underlying buffer (see next comment). Also, we don't need serialization for heap based RowIds. We can use something like BinaryRowId for byte array based row ids. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/IgniteRowId.java: ########## @@ -0,0 +1,30 @@ +/* + * 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; + +/** + * Interface that represents row id in primary index of the table. + * + * @see MvPartitionStorage + */ +public interface IgniteRowId extends Comparable<IgniteRowId> { Review Comment: 1. I would rename it to just RowId for consistency. Using the Ignite prefix must be consistent through the code, now seems it's used randomly. Whyd don't we use IgniteBinaryRow, for example ? 2. RowId is not required to be comparable. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/UuidIgniteRowId.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.UUID; +import org.jetbrains.annotations.NotNull; + +/** + * UUID-based ignite row id implementation. + */ +public final class UuidIgniteRowId implements IgniteRowId { + /* + * The most significant 64 bits. + */ + private final long mostSigBits; + + /* + * The least significant 64 bits. + */ + private final long leastSigBits; + + /** + * Constructor. + * + * @param uuid UUID. + */ + public UuidIgniteRowId(UUID uuid) { + this(uuid.getMostSignificantBits(), uuid.getLeastSignificantBits()); + } + + /** Private constructor. */ + private UuidIgniteRowId(long mostSigBits, long leastSigBits) { + this.mostSigBits = mostSigBits; + this.leastSigBits = leastSigBits; + } + + /** + * Returns {@link UuidIgniteRowId} instance based on {@link UUID#randomUUID()}. Highest two bytes of the most significant bits part will + * encode partition id. + * + * @param partitionId Partition id. + */ + public static IgniteRowId randomRowId(int partitionId) { Review Comment: This implementation allows duplicates. This must be clearly stated in javadoc. Better to use another UUID type, see https://datatracker.ietf.org/doc/draft-peabody-dispatch-new-uuid-format/, paragraph Distributed UUID Generation. UUID generation must be delegated to dedicated factory, not hard coded somethere in the code. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/UuidIgniteRowId.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.UUID; +import org.jetbrains.annotations.NotNull; + +/** + * UUID-based ignite row id implementation. + */ +public final class UuidIgniteRowId implements IgniteRowId { + /* + * The most significant 64 bits. + */ + private final long mostSigBits; + + /* + * The least significant 64 bits. + */ + private final long leastSigBits; + + /** + * Constructor. + * + * @param uuid UUID. + */ + public UuidIgniteRowId(UUID uuid) { + this(uuid.getMostSignificantBits(), uuid.getLeastSignificantBits()); + } + + /** Private constructor. */ + private UuidIgniteRowId(long mostSigBits, long leastSigBits) { + this.mostSigBits = mostSigBits; + this.leastSigBits = leastSigBits; + } + + /** + * Returns {@link UuidIgniteRowId} instance based on {@link UUID#randomUUID()}. Highest two bytes of the most significant bits part will + * encode partition id. + * + * @param partitionId Partition id. + */ + public static IgniteRowId randomRowId(int partitionId) { + UUID randomUuid = UUID.randomUUID(); + + long lsb = randomUuid.getLeastSignificantBits(); + + lsb = (lsb & ~0xFFFFL) | partitionId; + + return new UuidIgniteRowId(randomUuid.getMostSignificantBits(), lsb); + } + + /** {@inheritDoc} */ + @Override + public int partitionId() { + return (int) (leastSigBits & 0xFFFFL); + } + + /** + * Writes row id into a byte buffer. Binary row representation should match natural order defined by {@link #compareTo(Object)} when + * comparing lexicographically. + * + * @param buf Output byte buffer with {@link java.nio.ByteOrder#BIG_ENDIAN} byte order. + * @param signedBytesCompare Defines properties of a target binary comparator. {@code true} if bytes are compared as signed values, + * {@code false} if unsigned. + */ + public void writeTo(ByteBuffer buf, boolean signedBytesCompare) { + assert buf.order() == ByteOrder.BIG_ENDIAN; + + long mask = longBytesSignsMask(signedBytesCompare); + + buf.putLong(mask ^ mostSigBits); + buf.putLong(mask ^ leastSigBits); + } + + /** + * Compares row id with a byte buffer, previously written by a {@link #writeTo(ByteBuffer, boolean)} method. + * + * @param buf Input byte buffer with {@link java.nio.ByteOrder#BIG_ENDIAN} byte order. + * @param signedBytesCompare Defines properties of a binary comparator. {@code true} if bytes are compared as signed values, + * {@code false} if unsigned. + * @return A negative integer, zero, or a positive integer as this row id is less than, equal to, or greater than the specified row id. + */ + public int compareTo(ByteBuffer buf, boolean signedBytesCompare) { Review Comment: Can we use an underlying 16 byte buffer to store and work with high and low words? Right now this class looks like a poor copy&paste of an UUID. -- 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]
