FrankChen021 commented on code in PR #19548:
URL: https://github.com/apache/druid/pull/19548#discussion_r3981261387
##########
processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java:
##########
@@ -136,18 +139,51 @@ public String getName()
@Override
public ExtractionFn getExtractionFn()
{
- final LookupExtractor lookupExtractor;
+ final LookupExtractor lookupExtractor = getLookupExtractor();
+ return makeLookupExtractionFn(lookupExtractor);
+ }
+
+ @Override
+ public ExtractionFn getExtractionFnForMetadata()
Review Comment:
[P2] Use the metadata accessor for presence checks
The new getExtractionFnForMetadata() contract exists specifically because
capability checks discard the result without a close hook, but several existing
metadata-only checks still call getExtractionFn(): ListFilteredVirtualColumn
(including getIndexSupplier), ShimColumnSelectorFactory, and
SortMergeJoinFrameProcessor. For a named cached lookup these calls now acquire
and abandon a retained cache reference, even on paths that immediately reject
the query, unnecessarily pinning retired versions. Switch those presence checks
to getExtractionFnForMetadata().
##########
processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java:
##########
@@ -136,18 +139,51 @@ public String getName()
@Override
public ExtractionFn getExtractionFn()
{
- final LookupExtractor lookupExtractor;
+ final LookupExtractor lookupExtractor = getLookupExtractor();
+ return makeLookupExtractionFn(lookupExtractor);
+ }
+
+ @Override
+ public ExtractionFn getExtractionFnForMetadata()
+ {
+ return makeLookupExtractionFn(getLookupExtractorForMetadata());
+ }
+
+ private LookupExtractor getLookupExtractor()
+ {
+ if (Strings.isNullOrEmpty(name)) {
+ return this.lookup;
+ }
+
+ final LookupExtractorFactory lookupExtractorFactory =
getLookupExtractorFactory();
+ final Optional<RetainedLookupExtractor> retainedLookupExtractor =
Review Comment:
[P2] Avoid retaining a lookup for every joined row
LookupDimensionSpec.getExtractionFn() now acquires a retained cache
reference, but LookupColumnSelectorFactory calls it twice from getValue() for
every joined row (once for the null check and once for apply). The first
RetainedLookupExtractor is immediately discarded and the second is never
explicitly closed because the selector has no close hook. A lookup join that
projects an extraction on k/v can therefore accumulate Cleaner-managed
references, pin retired cache versions, and make CacheScheduler skip refreshes
until the timeout. Hoist one extraction function per selector and give its
retained resource an owner/lifetime; use the metadata accessor for the presence
check.
##########
server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java:
##########
@@ -109,10 +109,15 @@ public <T> QueryRunner<T>
getQueryRunnerForIntervals(final Query<T> query, final
.map(queryRunnerFactory::createRunner).iterator()
);
+ // close the segment map function when the result sequence is fully
consumed or closed, releasing resources it
+ // retains for the query, such as lookup versions pinned for joins
+ final QueryRunner<T> closingRunner =
+ (queryPlus, responseContext) -> baseRunner.run(queryPlus,
responseContext).withBaggage(segmentMapFn);
Review Comment:
[P2] Close the segment map function on query errors
The segmentMapFn is created before runner setup, but it is only attached as
sequence baggage after baseRunner.run() returns. If runner construction or
execution throws, the resource-bearing SegmentMapFunction and its retained
lookup joinables are not closed and must wait for Cleaner. Wrap setup/run
failures with an explicit close, matching the cleanup already added to
ServerManager and SinkQuerySegmentWalker.
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -384,49 +386,66 @@ public SegmentMapFunction createSegmentMapFunction(Query
query)
clauses,
joinableFactoryWrapper.getJoinableFactory()
);
- final JoinFilterRewriteConfig filterRewriteConfig =
JoinFilterRewriteConfig.forQuery(query);
-
- // Pick off any join clauses that can be converted into filters.
- final Set<String> requiredColumns = query.getRequiredColumns();
- final Filter baseFilterToUse;
- final List<JoinableClause> clausesToUse;
-
- if (requiredColumns != null &&
filterRewriteConfig.isEnableRewriteJoinToFilter()) {
- final Pair<List<Filter>, List<JoinableClause>> conversionResult =
JoinableFactoryWrapper.convertJoinsToFilters(
- joinableClauses.getJoinableClauses(),
- requiredColumns,
-
Ints.checkedCast(Math.min(filterRewriteConfig.getFilterRewriteMaxSize(),
Integer.MAX_VALUE))
- );
-
- baseFilterToUse = Filters.maybeAnd(
- Lists.newArrayList(
- Iterables.concat(
- Collections.singleton(baseFilter),
- conversionResult.lhs
- )
- )
- ).orElse(null);
- clausesToUse = conversionResult.rhs;
- } else {
- baseFilterToUse = baseFilter;
- clausesToUse = joinableClauses.getJoinableClauses();
- }
+ // joinables may retain resources (such as pinned lookup versions) that
are owned by the returned function's
+ // close(), if anything below throws before the function takes ownership,
release them here instead of leaving
+ // them to the Cleaner
+ try {
Review Comment:
[P2] Release joinables when clause construction fails
The cleanup try/catch starts after JoinableClauses.createClauses() returns.
That method builds clauses incrementally, so if a later
LookupJoinableFactory.build() or retained-reference acquisition fails, any
earlier LookupJoinable is lost without close(). The new retained lookup
reference can then block cache retirement and refreshes until Cleaner/timeout.
Make clause construction close already-built closeable joinables on failure, or
establish ownership before building the next clause.
--
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]
