imply-cheddar commented on code in PR #12646:
URL: https://github.com/apache/druid/pull/12646#discussion_r899694756
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorSequenceBuilder.java:
##########
@@ -50,51 +56,66 @@
import javax.annotation.Nullable;
import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
public class QueryableIndexCursorSequenceBuilder
{
private final QueryableIndex index;
private final Interval interval;
private final VirtualColumns virtualColumns;
- @Nullable
- private final ImmutableBitmap filterBitmap;
+ private final Filter filter;
+ private final QueryMetrics<? extends Query> metrics;
private final long minDataTimestamp;
private final long maxDataTimestamp;
private final boolean descending;
- @Nullable
- private final Filter postFilter;
- @Nullable
- private final ColumnSelectorColumnIndexSelector bitmapIndexSelector;
public QueryableIndexCursorSequenceBuilder(
QueryableIndex index,
Interval interval,
VirtualColumns virtualColumns,
- @Nullable ImmutableBitmap filterBitmap,
+ @Nullable Filter filter,
+ @Nullable QueryMetrics<? extends Query> metrics,
long minDataTimestamp,
long maxDataTimestamp,
- boolean descending,
- @Nullable Filter postFilter,
- @Nullable ColumnSelectorColumnIndexSelector bitmapIndexSelector
+ boolean descending
)
{
this.index = index;
this.interval = interval;
this.virtualColumns = virtualColumns;
- this.filterBitmap = filterBitmap;
+ this.filter = filter;
+ this.metrics = metrics;
this.minDataTimestamp = minDataTimestamp;
this.maxDataTimestamp = maxDataTimestamp;
this.descending = descending;
- this.postFilter = postFilter;
- this.bitmapIndexSelector = bitmapIndexSelector;
}
public Sequence<Cursor> build(final Granularity gran)
{
+ final Closer closer = Closer.create();
+
+ // Column caches shared amongst all cursors in this sequence.
+ final ColumnCache columnCache = new ColumnCache(index, closer);
+
final Offset baseOffset;
+ final ColumnSelectorColumnIndexSelector bitmapIndexSelector = new
ColumnSelectorColumnIndexSelector(
+ index.getBitmapFactoryForDimensions(),
+ virtualColumns,
+ columnCache
Review Comment:
(2) is not really an option at this point in time. The whole reason that
the column cache is being passed in is because there are a plethora of
legitimate bugs where we are not closing columns that need to be closed and
passing the columnCache is the way to fix that. Of course, we could go and
change all of the different places where that is broken, but that is a lot more
intrusive of a change.
(1) is possible, though, the real fundamental issue is that the `Closer`
pattern got added to the `Closeables` that are in the interfaces in Druid. The
addition of the `Closer` meant that we started losing track of where objects
are actually being closed as sometimes they are magically closed elsewhere and
sometimes they aren't. We likely need to just have a single pattern (either,
it's always via `Closeable`s or it's always via a `Closer`), but the goal of
this PR is to get us to where we proactively catch and fix leaks by failing
tests, so getting us to a place of consistency in how we deal with closing
objects is a bit too big of a thing.
That said, I think I can make it so that the cached Column objects from the
columnCache ignore their `close()` method so that the only way to close the
underlying object is to do it from the `Closer` which avoids the issue for now.
--
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]
