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/10/07 09:11:59 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
tkalkirill opened a new pull request, #1175:
URL: https://github.com/apache/ignite-3/pull/1175
https://issues.apache.org/jira/browse/IGNITE-17843
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990020863
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/BplusTree.java:
##########
@@ -113,8 +113,8 @@
* </code></pre>
* There are {@code N} items and {@code N+1} links. Each link points to page of a lower level. For example, pages on level {@code 2} always
* point to pages of level {@code 1}. For an item {@code i} left subtree is defined by {@code link(i)} and right subtree is defined by
- * {@code link(i+1)} ({@link BplusInnerIo#getLeft(long, int)} and {@link BplusInnerIo#getRight(long, int)}). All items in the left subtree
- * are less or equal to the original item (basic property for the trees).
+ * {@code link(i+1)} ({@link BplusInnerIo#getLeftPageId(long, int, int)} and {@link BplusInnerIo#getRightPageId(long, int, int)}).
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990091151
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusIo.java:
##########
@@ -346,14 +357,15 @@ public abstract void copyItems(
L row,
@Nullable byte[] rowBytes,
long rightId,
- boolean needRowBytes
+ boolean needRowBytes,
+ int partId
) throws IgniteInternalCheckedException {
assertPageType(pageAddr);
int cnt = getCount(pageAddr);
// Move right all the greater elements to make a free slot for a new row link.
- copyItems(pageAddr, pageAddr, idx, idx + 1, cnt - idx, false);
+ copyItems(pageAddr, pageAddr, idx, idx + 1, cnt - idx, false, partId);
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990039910
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -92,4 +100,45 @@ public static void writeToBuffer(ByteBuffer buffer, long link) {
buffer.putInt(pageIndex(link));
}
+
+ /**
+ * Reads a partitionless page ID from the memory.
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param partId Partition ID.
+ * @return Page ID with partition ID.
+ */
+ public static long readPartitionlessPageId(long addr, int off, int partId) {
Review Comment:
Fix it
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -92,4 +100,45 @@ public static void writeToBuffer(ByteBuffer buffer, long link) {
buffer.putInt(pageIndex(link));
}
+
+ /**
+ * Reads a partitionless page ID from the memory.
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param partId Partition ID.
+ * @return Page ID with partition ID.
+ */
+ public static long readPartitionlessPageId(long addr, int off, int partId) {
+ long rotationId = getByte(addr, off) & 0xFF;
+
+ byte flag = getByte(addr, off + Byte.BYTES);
+
+ int pageIdx = getInt(addr, off + Byte.BYTES + Byte.BYTES);
+
+ return pageId(partId, flag, pageIdx, rotationId);
+ }
+
+ /**
+ * Writes a partitionless page ID to memory.
+ * <ul>
+ * <li>{@link PageIdUtils#rotationId} - 1 byte;</li>
+ * <li>{@link PageIdUtils#flag Flag} - 1 byte;</li>
+ * <li>{@link PageIdUtils#pageIndex Page index} - 4 bytes.</li>
+ * </ul>
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param pageId Page ID.
+ * @return Number of bytes written (equal to {@link #PARTITIONLESS_PAGE_ID_SIZE_BYTES}).
+ */
+ public static long writePartitionlessPageId(long addr, int off, long pageId) {
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990020669
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/BplusTree.java:
##########
@@ -100,9 +100,9 @@
* <p>All pages in the tree are divided into levels. Leaves are always at the level {@code 0}. Levels of inner pages are thus positive.
* Each
* level represents a singly linked list - each page has a link to the <i>forward</i> page at the same level. It can be retrieved by calling
- * {@link BplusIo#getForward(long)}. This link must be a zero if there's no forward page. Forward links on level {@code 0} allow iterating
- * tree's keys and values effectively without traversing any inner nodes ({@code AbstractForwardCursor}). Forward links in inner nodes have
- * different purpose, more on that later.
+ * {@link BplusIo#getForwardPageId(long, int)}. This link must be a zero if there's no forward page. Forward links on level {@code 0} allow
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990058155
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
- assert pageId == getLeft(pageAddr, idx);
+ assert pageId == getLeft(pageAddr, idx, partitionId(pageId));
}
/**
- * Returns right page id for item.
+ * Returns right page ID for item.
Review Comment:
Fix it
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
- assert pageId == getLeft(pageAddr, idx);
+ assert pageId == getLeft(pageAddr, idx, partitionId(pageId));
}
/**
- * Returns right page id for item.
+ * Returns right page ID for item.
*
* @param pageAddr Page address.
* @param idx Index of item.
+ * @param partId Partition ID.
*/
- public final long getRight(long pageAddr, int idx) {
- return getLong(pageAddr, offset0(idx, shiftRight));
+ public final long getRight(long pageAddr, int idx, int partId) {
+ return readPartitionless(partId, pageAddr, offset0(idx, shiftRight));
}
/**
- * Returns right page id for item.
+ * Sets right page ID for item.
Review Comment:
fix it
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990055989
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
Review Comment:
Fix it
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990051723
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
- assert pageId == getLeft(pageAddr, idx);
+ assert pageId == getLeft(pageAddr, idx, partitionId(pageId));
}
/**
- * Returns right page id for item.
+ * Returns right page ID for item.
*
* @param pageAddr Page address.
* @param idx Index of item.
+ * @param partId Partition ID.
*/
- public final long getRight(long pageAddr, int idx) {
- return getLong(pageAddr, offset0(idx, shiftRight));
+ public final long getRight(long pageAddr, int idx, int partId) {
+ return readPartitionless(partId, pageAddr, offset0(idx, shiftRight));
}
/**
- * Returns right page id for item.
+ * Sets right page ID for item.
Review Comment:
Same here, please leave "id"
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -29,25 +29,26 @@
import java.nio.ByteBuffer;
/**
- * Handling of <em>partitionless links</em>, that is, page memory links from which partition ID is removed.
+ * Auxiliary class for working with links and page IDs without partition ID.
*
* <p>They are used to save storage space in cases when we know the partition ID from the context.
*
* @see PageIdUtils#link(long, int)
+ * @see PageIdUtils#pageId(int, byte, int)
*/
public class PartitionlessLinks {
/** Number of bytes a partitionless link takes in storage. */
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Reads a partitionless link from the memory.
+ * Reads a partitionless link or page ID from the memory.
*
* @param partitionId Partition ID.
* @param pageAddr Page address.
* @param offset Data offset.
- * @return Partitionless link.
+ * @return Link or page ID with partition ID.
*/
- public static long readPartitionlessLink(int partitionId, long pageAddr, int offset) {
+ public static long readPartitionless(int partitionId, long pageAddr, int offset) {
Review Comment:
I also asked to update comment in this method. Line `Page with index 0 is never a data page.` makes no sense now, it should be changed
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusIo.java:
##########
@@ -346,14 +357,15 @@ public abstract void copyItems(
L row,
@Nullable byte[] rowBytes,
long rightId,
- boolean needRowBytes
+ boolean needRowBytes,
+ int partId
) throws IgniteInternalCheckedException {
assertPageType(pageAddr);
int cnt = getCount(pageAddr);
// Move right all the greater elements to make a free slot for a new row link.
- copyItems(pageAddr, pageAddr, idx, idx + 1, cnt - idx, false);
+ copyItems(pageAddr, pageAddr, idx, idx + 1, cnt - idx, false, partId);
Review Comment:
`partId` doesn't seem necessary for copying, why is it here?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
- assert pageId == getLeft(pageAddr, idx);
+ assert pageId == getLeft(pageAddr, idx, partitionId(pageId));
}
/**
- * Returns right page id for item.
+ * Returns right page ID for item.
Review Comment:
Please don't capitalize it. First of all, why would you touch a comment that has nothing to do with your changes. Second, "id" is grammatically correct.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/io/BplusInnerIo.java:
##########
@@ -81,23 +84,24 @@ public final long getLeft(long pageAddr, int idx) {
public final void setLeft(long pageAddr, int idx, long pageId) {
assertPageType(pageAddr);
- putLong(pageAddr, offset0(idx, SHIFT_LEFT), pageId);
+ writePartitionless(pageAddr + offset0(idx, SHIFT_LEFT), link(pageId, 0));
Review Comment:
I believe that having a `link(pageId, 0)` is wrong here.
--
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
[GitHub] [ignite-3] tkalkirill merged pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill merged PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990013201
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/BplusTree.java:
##########
@@ -100,9 +100,9 @@
* <p>All pages in the tree are divided into levels. Leaves are always at the level {@code 0}. Levels of inner pages are thus positive.
* Each
* level represents a singly linked list - each page has a link to the <i>forward</i> page at the same level. It can be retrieved by calling
- * {@link BplusIo#getForward(long)}. This link must be a zero if there's no forward page. Forward links on level {@code 0} allow iterating
- * tree's keys and values effectively without traversing any inner nodes ({@code AbstractForwardCursor}). Forward links in inner nodes have
- * different purpose, more on that later.
+ * {@link BplusIo#getForwardPageId(long, int)}. This link must be a zero if there's no forward page. Forward links on level {@code 0} allow
Review Comment:
Old name was good enough, why did you rename it? Please change it back.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/tree/BplusTree.java:
##########
@@ -113,8 +113,8 @@
* </code></pre>
* There are {@code N} items and {@code N+1} links. Each link points to page of a lower level. For example, pages on level {@code 2} always
* point to pages of level {@code 1}. For an item {@code i} left subtree is defined by {@code link(i)} and right subtree is defined by
- * {@code link(i+1)} ({@link BplusInnerIo#getLeft(long, int)} and {@link BplusInnerIo#getRight(long, int)}). All items in the left subtree
- * are less or equal to the original item (basic property for the trees).
+ * {@code link(i+1)} ({@link BplusInnerIo#getLeftPageId(long, int, int)} and {@link BplusInnerIo#getRightPageId(long, int, int)}).
Review Comment:
Same for these two. Please also return the old formatting back after renaming, if it fits
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -92,4 +100,45 @@ public static void writeToBuffer(ByteBuffer buffer, long link) {
buffer.putInt(pageIndex(link));
}
+
+ /**
+ * Reads a partitionless page ID from the memory.
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param partId Partition ID.
+ * @return Page ID with partition ID.
+ */
+ public static long readPartitionlessPageId(long addr, int off, int partId) {
+ long rotationId = getByte(addr, off) & 0xFF;
+
+ byte flag = getByte(addr, off + Byte.BYTES);
+
+ int pageIdx = getInt(addr, off + Byte.BYTES + Byte.BYTES);
+
+ return pageId(partId, flag, pageIdx, rotationId);
+ }
+
+ /**
+ * Writes a partitionless page ID to memory.
+ * <ul>
+ * <li>{@link PageIdUtils#rotationId} - 1 byte;</li>
+ * <li>{@link PageIdUtils#flag Flag} - 1 byte;</li>
+ * <li>{@link PageIdUtils#pageIndex Page index} - 4 bytes.</li>
+ * </ul>
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param pageId Page ID.
+ * @return Number of bytes written (equal to {@link #PARTITIONLESS_PAGE_ID_SIZE_BYTES}).
+ */
+ public static long writePartitionlessPageId(long addr, int off, long pageId) {
Review Comment:
Remove this method as well
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -92,4 +100,45 @@ public static void writeToBuffer(ByteBuffer buffer, long link) {
buffer.putInt(pageIndex(link));
}
+
+ /**
+ * Reads a partitionless page ID from the memory.
+ *
+ * @param addr Address in memory.
+ * @param off Offset from {@code addr} in bytes.
+ * @param partId Partition ID.
+ * @return Page ID with partition ID.
+ */
+ public static long readPartitionlessPageId(long addr, int off, int partId) {
Review Comment:
What's the difference between this and `readPartitionlessLink`?
There should be no difference. Not only this implementation is buggy, it's also a copy-paste.
Please make a single method and update comment in it. Renaming here would be fine
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1175: IGNITE-17843 Use partitionless pageId in BplusInnerIo
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1175:
URL: https://github.com/apache/ignite-3/pull/1175#discussion_r990073414
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PartitionlessLinks.java:
##########
@@ -29,25 +29,26 @@
import java.nio.ByteBuffer;
/**
- * Handling of <em>partitionless links</em>, that is, page memory links from which partition ID is removed.
+ * Auxiliary class for working with links and page IDs without partition ID.
*
* <p>They are used to save storage space in cases when we know the partition ID from the context.
*
* @see PageIdUtils#link(long, int)
+ * @see PageIdUtils#pageId(int, byte, int)
*/
public class PartitionlessLinks {
/** Number of bytes a partitionless link takes in storage. */
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Reads a partitionless link from the memory.
+ * Reads a partitionless link or page ID from the memory.
*
* @param partitionId Partition ID.
* @param pageAddr Page address.
* @param offset Data offset.
- * @return Partitionless link.
+ * @return Link or page ID with partition ID.
*/
- public static long readPartitionlessLink(int partitionId, long pageAddr, int offset) {
+ public static long readPartitionless(int partitionId, long pageAddr, int offset) {
Review Comment:
fix it
--
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