You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/06/03 10:14:27 UTC
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #847: IGNITE-17009 Support fragmented RowVersions
sashapolo commented on code in PR #847:
URL: https://github.com/apache/ignite-3/pull/847#discussion_r888771461
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/RowVersionDataIo.java:
##########
@@ -60,25 +60,54 @@ protected void writeRowData(long pageAddr, int dataOff, int payloadSize, RowVers
putShort(addr, 0, (short) payloadSize);
addr += 2;
- addr += Timestamps.writeTimestamp(addr, 0, row.timestamp());
+ addr += Timestamps.writeTimestampToMemory(addr, 0, row.timestamp());
addr += PartitionlessLinks.writeToMemory(addr, row.nextLink());
- ByteBuffer value = row.value();
- putInt(addr, 0, value.limit());
+ putInt(addr, 0, row.valueSize());
addr += 4;
- putByteBuffer(addr, 0, value);
+ putByteBuffer(addr, 0, row.value());
}
/** {@inheritDoc} */
@Override
protected void writeFragmentData(RowVersion row, ByteBuffer buf, int rowOff, int payloadSize) {
assertPageType(buf);
- // TODO: IGNITE-17009 - implement
+ if (rowOff == 0) {
+ // first fragment
+ assert row.headerSize() <= payloadSize : "Header must entirely fit first fragment, but header size is " + row.headerSize()
Review Comment:
```suggestion
assert row.headerSize() <= payloadSize : "Header must entirely fit in the first fragment, but header size is " + row.headerSize()
```
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/datapage/ReadPageMemoryRowValue.java:
##########
@@ -86,8 +86,10 @@ private long readFully(long pageAddr, DataPagePayload payload) {
}
private void readValueFragmentToArray(long pageAddr, DataPagePayload payload, int offsetToValue) {
- PageUtils.getBytes(pageAddr, payload.offset() + offsetToValue, allValueBytes, transferredBytes, payload.payloadSize());
- transferredBytes += payload.payloadSize();
+ int valueBytesToRead = payload.payloadSize() - offsetToValue;
Review Comment:
Shall we add an assertion that `allValueBytes != null`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/RowVersionDataIo.java:
##########
@@ -60,25 +60,54 @@ protected void writeRowData(long pageAddr, int dataOff, int payloadSize, RowVers
putShort(addr, 0, (short) payloadSize);
addr += 2;
- addr += Timestamps.writeTimestamp(addr, 0, row.timestamp());
+ addr += Timestamps.writeTimestampToMemory(addr, 0, row.timestamp());
addr += PartitionlessLinks.writeToMemory(addr, row.nextLink());
- ByteBuffer value = row.value();
- putInt(addr, 0, value.limit());
+ putInt(addr, 0, row.valueSize());
addr += 4;
- putByteBuffer(addr, 0, value);
+ putByteBuffer(addr, 0, row.value());
}
/** {@inheritDoc} */
@Override
protected void writeFragmentData(RowVersion row, ByteBuffer buf, int rowOff, int payloadSize) {
Review Comment:
Should we validate the `rowOff` against the `row` capacity and `payloadSize` against the `buf` capacity?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/Timestamps.java:
##########
@@ -64,6 +65,19 @@ public static int writeTimestamp(long addr, int offset, @Nullable Timestamp time
return 2 * Long.BYTES;
}
+ /**
+ * Writes a {@link Timestamp} to a buffer.
+ *
+ * @param buffer buffer to which to write
+ * @param timestamp the timestamp to write
+ */
+ public static void writeTimestampToBuffer(ByteBuffer buffer, @Nullable Timestamp timestamp) {
Review Comment:
I wonder why aren't these classes present in the `Timestamp` class itself?
--
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