nozjkoitop commented on code in PR #19548:
URL: https://github.com/apache/druid/pull/19548#discussion_r3380570493
##########
server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java:
##########
@@ -57,9 +61,20 @@ public Optional<Joinable> build(final DataSource dataSource,
final JoinCondition
if (condition.canHashJoin()) {
final String lookupName = lookupDataSource.getLookupName();
return lookupProvider.get(lookupName)
- .map(c ->
LookupJoinable.wrap(c.getLookupExtractorFactory().get()));
+ .map(this::buildLookupJoinable);
} else {
return Optional.empty();
}
}
+
+ private LookupJoinable buildLookupJoinable(final
LookupExtractorFactoryContainer container)
+ {
+ final LookupExtractorFactory lookupExtractorFactory =
container.getLookupExtractorFactory();
+ final Optional<RetainedLookupExtractor> retainedLookupExtractor =
+ lookupExtractorFactory.acquireRetainedLookupExtractor();
Review Comment:
Yeah, `acquireReference()` is per mapped segment, but the pinned lookup
lives at the joinable/query level, so closing it there is too narrow and can be
wrong if the joinable is reused.
Deterministic cleanup would need a real close hook for the
Joinable/JoinableClause lifecycle, not just wiring the retained handle into
`LookupJoinable.acquireReference()`, which will mean the join api change and
I'm not sure if we want it under this pull request, would it be reasonable to
leave the Cleaner fallback here and handle deterministic cleanup as a follow up?
--
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]
