You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "Tan-JiaLiang (via GitHub)" <gi...@apache.org> on 2024/12/24 05:49:07 UTC
[PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Tan-JiaLiang opened a new pull request, #4765:
URL: https://github.com/apache/paimon/pull/4765
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### Purpose
<!-- Linking this pull request to the issue -->
In `RoaringBitmap`, the `deserialize(java.io.DataInput, byte[])` function has a better performance to deserialize the `BitmapContainer`.
<!-- What is the purpose of the change -->
### Tests
<!-- List UT and IT cases to verify this change -->
No.
### API and Format
<!-- Does this change affect API or storage format -->
No.
### Documentation
<!-- Does this change introduce a new feature -->
No.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1897062775
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
Did you mean adding a benchmark?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2560767882
Can you provide the benchmark result?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2560895187
I'm trying to understand how to use the benchmark in paimon, and I hope my benchmark using in correct.
bitmap32-deserialize-benchmark | Best/Avg Time(ms) | Row Rate(K/s) | Per Row(ns) | Relative
-------- | ----- | ----- | ----- | -----
bitmap32-deserialize-benchmark_without-buffer | 239 / 259 | 0.4 | 2385027.0 | 1.0X
bitmap32-deserialize-benchmark_with-buffer | 2 / 2 | 54.1 | 18500.0 | 128.9X
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2563544139
@JingsongLi @Zouxxyy Can you help to review again?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1898547151
##########
paimon-core/src/main/java/org/apache/paimon/deletionvectors/DeletionVector.java:
##########
@@ -99,11 +99,11 @@ default boolean checkedDelete(long position) {
* @return A DeletionVector instance that represents the deserialized data.
*/
static DeletionVector deserializeFromBytes(byte[] bytes) {
- try (ByteArrayInputStream bis = new ByteArrayInputStream(bytes);
- DataInputStream dis = new DataInputStream(bis)) {
- int magicNum = dis.readInt();
+ try {
+ ByteBuffer buffer = ByteBuffer.wrap(bytes);
+ int magicNum = buffer.getInt();
if (magicNum == BitmapDeletionVector.MAGIC_NUMBER) {
- return BitmapDeletionVector.deserializeFromDataInput(dis);
+ return BitmapDeletionVector.deserializeFromByteBuffer(buffer.slice());
Review Comment:
This is really just a programming convention to ensure that the `ByteBuffer` passed in are bitmaps only. I take a look that it is already using in `RoaringArray#deserialize`, so it's redundant here.
https://github.com/RoaringBitmap/RoaringBitmap/blob/60dcdb58846037ea8ea712317a0ac887e00fe25b/roaringbitmap/src/main/java/org/roaringbitmap/RoaringArray.java#L544-L550
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2561563157
Add the `deserialize(ByteBuffer)` benchmark
bitmap32-deserialize-benchmark | Best/Avg Time(ms) | Row Rate(K/s) | Per Row(ns) | Relative
-- | -- | -- | -- | --
benchmark_deserialize(DataInput) | 242 / 254 | 0.4 | 2417750.0 | 1.0X
benchmark_deserialize(DataInput, byte[]) | 2 / 3 | 49.8 | 20087.0 | 120.4X
benchmark_deserialize(ByteBuffer) | 1 / 2 | 75.3 | 13287.0 | 182.0X
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1896644552
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -89,6 +91,10 @@ public void deserialize(DataInput in) throws IOException {
roaringBitmap.deserialize(in);
}
+ public void deserialize(DataInput in, @Nullable byte[] buffer) throws IOException {
+ roaringBitmap.deserialize(in, buffer);
Review Comment:
I totally agree with this, but be aware that it may take up 8KB of memory during deserialization, although I think it's reasonable.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.
Zouxxyy commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1897073314
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
From the test results, it is more optimal, and I have checked other formats, which also use it. we can just use 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2563816725
Seems the CDC e2e test is still having some thread safe problem, I will take a look in next week. Can someone help to trigger the build one more time?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1898547151
##########
paimon-core/src/main/java/org/apache/paimon/deletionvectors/DeletionVector.java:
##########
@@ -99,11 +99,11 @@ default boolean checkedDelete(long position) {
* @return A DeletionVector instance that represents the deserialized data.
*/
static DeletionVector deserializeFromBytes(byte[] bytes) {
- try (ByteArrayInputStream bis = new ByteArrayInputStream(bytes);
- DataInputStream dis = new DataInputStream(bis)) {
- int magicNum = dis.readInt();
+ try {
+ ByteBuffer buffer = ByteBuffer.wrap(bytes);
+ int magicNum = buffer.getInt();
if (magicNum == BitmapDeletionVector.MAGIC_NUMBER) {
- return BitmapDeletionVector.deserializeFromDataInput(dis);
+ return BitmapDeletionVector.deserializeFromByteBuffer(buffer.slice());
Review Comment:
This is really just a programming convention to ensure that the bytes passed in are bitmaps only. I take a look that it is already using in `RoaringArray#deserialize`, so it's redundant here.
https://github.com/RoaringBitmap/RoaringBitmap/blob/60dcdb58846037ea8ea712317a0ac887e00fe25b/roaringbitmap/src/main/java/org/roaringbitmap/RoaringArray.java#L544-L550
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1896608377
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -89,6 +91,10 @@ public void deserialize(DataInput in) throws IOException {
roaringBitmap.deserialize(in);
}
+ public void deserialize(DataInput in, @Nullable byte[] buffer) throws IOException {
+ roaringBitmap.deserialize(in, buffer);
Review Comment:
if `deserialize(in, null)` is better, can you just modify `RoaringBitmap32.deserialize(DataInput)`?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1897086712
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
We can using it in delection vector bitmap and rbm aggregate function, but the bitmap and bsi index can not for now.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #4765:
URL: https://github.com/apache/paimon/pull/4765
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #4765:
URL: https://github.com/apache/paimon/pull/4765#issuecomment-2560920219
Thanks for your information.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.
Zouxxyy commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1896726913
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
Maybe we can tested `roaringBitmap.deserialize(ByteBuffer bbf)` directly? We actually know the exact byte count of the bitmap
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Tan-JiaLiang (via GitHub)" <gi...@apache.org>.
Tan-JiaLiang commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1897082080
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
The premise of ByteBuffer is that we need to know the actual size of the Bitmap. This seems to require recording the actual size at serialisation time.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Add a faster deserialize function to RoaringBitmap32 [paimon]
Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.
Zouxxyy commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1898221438
##########
paimon-common/src/main/java/org/apache/paimon/utils/RoaringBitmap32.java:
##########
@@ -86,7 +86,7 @@ public void serialize(DataOutput out) throws IOException {
}
public void deserialize(DataInput in) throws IOException {
- roaringBitmap.deserialize(in);
+ roaringBitmap.deserialize(in, null);
Review Comment:
> roaringBitmap.deserialize(ByteBuffer bbf)
Does the bitmap index not record the byte size? Would you be willing to integrate `roaringBitmap.deserialize(ByteBuffer bbf)` into a usable place such as dv?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Using a faster deserialization method in RoaringBitmap32 [paimon]
Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.
Zouxxyy commented on code in PR #4765:
URL: https://github.com/apache/paimon/pull/4765#discussion_r1898472350
##########
paimon-core/src/main/java/org/apache/paimon/deletionvectors/DeletionVector.java:
##########
@@ -99,11 +99,11 @@ default boolean checkedDelete(long position) {
* @return A DeletionVector instance that represents the deserialized data.
*/
static DeletionVector deserializeFromBytes(byte[] bytes) {
- try (ByteArrayInputStream bis = new ByteArrayInputStream(bytes);
- DataInputStream dis = new DataInputStream(bis)) {
- int magicNum = dis.readInt();
+ try {
+ ByteBuffer buffer = ByteBuffer.wrap(bytes);
+ int magicNum = buffer.getInt();
if (magicNum == BitmapDeletionVector.MAGIC_NUMBER) {
- return BitmapDeletionVector.deserializeFromDataInput(dis);
+ return BitmapDeletionVector.deserializeFromByteBuffer(buffer.slice());
Review Comment:
Why is slice needed 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org