javanna commented on code in PR #16091:
URL: https://github.com/apache/lucene/pull/16091#discussion_r3414165643
##########
lucene/join/src/java/org/apache/lucene/search/join/JoinUtil.java:
##########
@@ -550,11 +554,26 @@ public static Query createJoinQuery(
break;
case None:
if (min <= 1 && max == Integer.MAX_VALUE) {
- GlobalOrdinalsCollector globalOrdinalsCollector =
- new GlobalOrdinalsCollector(joinField, ordinalMap, valueCount);
- searcher.search(rewrittenFromQuery, globalOrdinalsCollector);
+ LongBitSet collectedOrds =
+ searcher.search(
+ rewrittenFromQuery,
+ new CollectorManager<GlobalOrdinalsCollector, LongBitSet>() {
+ @Override
+ public GlobalOrdinalsCollector newCollector() {
+ return new GlobalOrdinalsCollector(joinField,
finalOrdinalMap, valueCount);
+ }
Review Comment:
Going back to this conversation.
On the memory per slice:
The current design allocates one `LongBitSet(segmentValueCount)` per
`getLeafCollector()` call. With intra-segment partitioning, a segment split
into K slices → K collectors → K separate `LongBitSet(segmentValueCount)`
instances. Those get OR-ed in reduce() via `mergedBySegment`.
The original used a single `LongBitSet(globalValueCount)` — that's the full
global cardinality. Each segment-local bitset is smaller (segment ordinals, not
global). Even K of them can be smaller than one global bitset when K is small
and there are many segments.
On the extra work from deferred remap:
It's potentially an improvement. The original remapped every matching
document (O(matching docs)), whereas the new code remaps unique set bits in
reduce (O(unique ordinals hit)). In `reduce`, `mergedBySegment.merge()` first
consolidates multiple collectors for the same segment with OR, then calls
`getGlobalOrds(segOrd)` exactly once per unique segment — that's the same as
the original.
I'd like to move this forward and this would help make progress with
removing `search(Query, Collector)` soon. I find the current changes reasonable
with this goal in mind, and we can always improve things performance wise later
if needed.
--
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]
