Core: Make PositionDeleteIndex serializable #11463
Conversation
| * magic bytes and bitmap for compatibility with Delta. | ||
| */ | ||
| @Override | ||
| public ByteBuffer serialize() { |
There was a problem hiding this comment.
Puffin writers currently assume we will create and pass ByteBuffer. We also compute a checksum on serialized magic bytes and bitmap, so I am allocating a byte array upfront.
| @@ -43,6 +53,11 @@ class BitmapPositionDeleteIndex implements PositionDeleteIndex { | |||
| this.deleteFiles = deleteFile != null ? Lists.newArrayList(deleteFile) : Lists.newArrayList(); | |||
| } | |||
|
|
|||
| BitmapPositionDeleteIndex(RoaringPositionBitmap bitmap, DeleteFile deleteFile) { | |||
| this.bitmap = bitmap; | |||
| this.deleteFiles = deleteFile != null ? Lists.newArrayList(deleteFile) : Lists.newArrayList(); | |||
There was a problem hiding this comment.
Is there a case where we can have a deserialized bitmap, but no delete file? Just wondering if we should have a separate constructor or if it makes sense to allow passing null for those cases.
There was a problem hiding this comment.
I don't think so. I have this check here to match the constructor above. There are some legacy code paths that create an index and pass null as DeleteFile. Given that it is a new constructor, we can probably require the file to be non-null, but it won't be consistent with the constructor above.
| */ | ||
| @Override | ||
| public ByteBuffer serialize() { | ||
| bitmap.runLengthEncode(); // run-length encode the bitmap before serializing |
There was a problem hiding this comment.
Should we more eagerly run length encode the bitmap in the constructor instead of at serialization time?
There was a problem hiding this comment.
NVM, I don't think it'll make any difference and probably more readable the way it's written at the moment.
There was a problem hiding this comment.
We actually want to accumulate all values prior to applying run-length encoding, so doing this right before the serialization seems like a good place. We don't anticipate the bitmap to change after serialization.
| long cardinality = bitmap.cardinality(); | ||
| long expectedCardinality = deleteFile.recordCount(); |
There was a problem hiding this comment.
NIt: I feel like we could inline bitMap.cardinality() == deleteFile.recordCount() in the check below and the intent of the check would still be clear
There was a problem hiding this comment.
Did this for consistency with the method above. It shouldn't be expensive to call recordCount() twice, so I am OK changing it.
There was a problem hiding this comment.
Let me actually keep it to avoid one more round of CI.
|
Thanks for reviewing, @danielcweeks @amogh-jahagirdar! |
This PR makes
PositionDeleteIndexserializable as per the V3 spec.This work is part of #11122.