gortiz commented on PR #19587: URL: https://github.com/apache/pinot/pull/19587#issuecomment-5762955194
Really nice work on the measurements here — the profiling, the A/B tables and the ownership/interleaving validation are all more rigorous than the average perf PR. My concern isn't whether this works; I traced the lazy contract through every escape point (aggregation holders, the on-heap star-tree builder, realtime pre-aggregation, both JSON readers) and found no correctness gap. It's about where the code lives. The PR puts three classes inside `org.roaringbitmap` / `org.roaringbitmap.buffer` but ships them from `pinot-common`. That's a split package against a library with a `module-info`, built on `protected` and package-private members — including `RoaringArray.mergeBulk` and `MERGE_OR`, which didn't exist before 1.6.16. Once merged, those shims tend to become permanent, and every RoaringBitmap bump becomes a silent-wrong-results risk rather than a compile error. **The good news:** upstream already ships this exact escape hatch. `RoaringBitmapPrivate` and `MutableRoaringBitmapPrivate` have been there since >= 1.3.0 (they're in our pinned 1.6.23), with the javadoc *"This class enables accessing/executing not-public methods."* They already export `repairAfterLazy` and `naivelazyor` — they're just missing `lazyor`. So the upstream ask is six lines: ```java // RoaringBitmapPrivate public static void lazyor(RoaringBitmap x1, RoaringBitmap x2) { x1.lazyor(x2); } // MutableRoaringBitmapPrivate — ImmutableRoaringBitmap, unlike their naivelazyor wrapper public static void lazyor(MutableRoaringBitmap x1, ImmutableRoaringBitmap x2) { x1.lazyor(x2); } ``` And your own rationale for rejecting `naivelazyor` (8 KiB promotion on first overlap, hundreds of MB transient for hash-spread bitmaps) *is* the justification for that upstream PR — it's a concrete gap in an API they already maintain for this purpose. `RoaringBitmapMerge` is a different case: it's an algorithm, not an access shim, so no `*Private` facade helps. But it belongs upstream too — `RoaringBitmap.or()` in 1.6.23 is already the same shape (linear scan -> `mergeBulk` on first source-only key -> `appendCopy` tail), so the delta is swapping `pos1++` for `advanceUntil` plus the singleton insert. That's a contained patch to a method they reworked in 1.6.16, it helps every Roaring user with a big-accumulator/small-input fold, and you already have the numbers to justify it. **Proposal:** hold this PR and file those two upstream. If the accessor lands, the lazy-union half (star-tree, JSON index, MV count, leaf aggregation) comes back here nearly unchanged. If the merge patch lands, `RoaringBitmapMerge` disappears entirely. If upstream declines the merge patch, let's revisit it as a deliberate, labelled stopgap — with a version-pin comment, a randomized `RoaringBitmapMerge.or` vs `RoaringBitmap.or` equivalence test, and a JMH benchmark for `merge()` (it's the change with the largest claimed win and currently the only one without one). -- 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]
