Jackie-Jiang commented on code in PR #19914:
URL: https://github.com/apache/pinot/pull/19914#discussion_r4300854156
##########
pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java:
##########
@@ -1366,20 +1367,33 @@ public Geometry deserialize(ByteBuffer byteBuffer) {
@Override
public int getSerializedSize(RoaringBitmap bitmap) {
+ // Run containers allocate typed views when writing directly, so the
heap serializer is cheaper.
+ if (bitmap.hasRunCompression()) {
+ return -1;
Review Comment:
**[P2] Bound the heap fallback by payload size**
`hasRunCompression()` is true when any container is a run container, so this
also disables direct serialization for large mixed bitmaps. For example, one
container holding a contiguous 32,000-value run plus 127 dense containers
holding alternating values has a 1,041,434-byte serialized payload. Returning
`-1` makes the companion Arrow writer allocate that entire temporary array and
then copy it into Arrow for every value; the same writer with this PR's base
codec writes directly using fixed-size container views. This adds roughly 1 MB
of temporary allocation and copying per value, despite the benefit for the
small run-only benchmark. Please limit the fallback to payloads where the
allocation savings justify the copy, preserve direct serialization for large
mixed bitmaps, and include a mixed-container case in the performance comparison.
--
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]
