xiangfu0 commented on code in PR #19836:
URL: https://github.com/apache/pinot/pull/19836#discussion_r4278289789
##########
pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java:
##########
@@ -1240,9 +1323,28 @@ public Geometry deserialize(ByteBuffer byteBuffer) {
public static final ObjectSerDe<RoaringBitmap> ROARING_BITMAP_SER_DE = new
ObjectSerDe<RoaringBitmap>() {
+ @Override
+ public int getSerializedSize(RoaringBitmap bitmap) {
+ int size = bitmap.serializedSizeInBytes();
Review Comment:
`serializedSizeInBytes()` ends up walking all containers more than once per
serialize: the caller calls `getSerializedSize` to allocate, then `serialize`
calls it again inside `checkSerializationBuffer`. For large bitmaps on a hot
path it may be worth caching the size the caller already computed, or at least
worth a note. Not blocking.
##########
pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java:
##########
@@ -323,11 +326,35 @@ public static ObjectType getObjectType(Object value) {
/// Serializer/De-serializer for a specific type of object.
///
+ /// Implementations must override either the byte-array serializer or both
[#getSerializedSize(Object)] and
+ /// the buffer serializer [#serialize(Object, ByteBuffer)].
+ ///
/// @param <T> Type of the object
public interface ObjectSerDe<T> {
/// Serializes a value into a byte array.
- byte[] serialize(T value);
+ /// Typed forwarding overrides preserve receiver specialization when
invoking this default.
+ default byte[] serialize(T value) {
+ byte[] bytes = new byte[getSerializedSize(value)];
+ serialize(value, ByteBuffer.wrap(bytes));
+ return bytes;
+ }
+
+ /// Returns the exact size of a representable payload, or `-1` when
allocation-free sizing is unsupported.
+ /// This method must not serialize the value or allocate a payload array.
The value must remain unchanged between
+ /// sizing and serialization. The destination must be bounded to this size
when serializing.
+ default int getSerializedSize(T value) {
+ return -1;
+ }
+
+ /// Writes the canonical payload at the destination's current position and
advances it by the payload size.
+ /// Supports heap and direct buffers, preserves the destination's limit
and byte order, and does not retain the
+ /// buffer. Direct-writing overrides must not mutate the value. A
read-only destination or insufficient remaining
+ /// space fails before any write. The default implementation preserves the
legacy serializer behavior and
+ /// serializes once into a temporary array; codecs can override it for
direct writes.
+ default void serialize(T value, ByteBuffer destination) {
Review Comment:
The two defaults call each other, so a codec that overrides neither
serializer would recurse infinitely. The Javadoc states the contract clearly,
which is probably enough, but a cheap guard (or just noting it's enforced only
by convention) could save a future implementer. Minor.
##########
pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java:
##########
@@ -1240,9 +1323,28 @@ public Geometry deserialize(ByteBuffer byteBuffer) {
public static final ObjectSerDe<RoaringBitmap> ROARING_BITMAP_SER_DE = new
ObjectSerDe<RoaringBitmap>() {
+ @Override
+ public int getSerializedSize(RoaringBitmap bitmap) {
+ int size = bitmap.serializedSizeInBytes();
+ Preconditions.checkArgument(size >= 0, "Serialized bitmap exceeds the
maximum buffer size");
+ return size;
+ }
+
@Override
public byte[] serialize(RoaringBitmap bitmap) {
- return RoaringBitmapUtils.serialize(bitmap);
+ return ObjectSerDe.super.serialize(bitmap);
+ }
+
+ @Override
+ public void serialize(RoaringBitmap bitmap, ByteBuffer destination) {
+ checkSerializationBuffer(destination, getSerializedSize(bitmap));
+ ByteOrder originalOrder = destination.order();
+ destination.order(ByteOrder.LITTLE_ENDIAN);
Review Comment:
Nice that this keeps the little-endian order. Worth a one-line comment that
RoaringBitmap's canonical format is little-endian and it restores the buffer
order itself, so this explicit set mainly documents intent and guards callers
who hand us a big-endian buffer.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
