clintropolis commented on code in PR #12646:
URL: https://github.com/apache/druid/pull/12646#discussion_r896379882
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndex.java:
##########
@@ -35,7 +38,7 @@
* @see QueryableIndexStorageAdapter for query path adapter
* @see QueryableIndexIndexableAdapter for indexing path adapter
*/
-public interface QueryableIndex extends ColumnSelector, Closeable
+public interface QueryableIndex extends Closeable
Review Comment:
since this still implements `getColumnCapabilities` should it still extend
`ColumnInspector`? it might save some of the churn of making a column cache for
things that only needed to get capabilities (like expressions trying to compute
their output type). Otoh it doesn't really seem necessary either, mostly just
wondering if it was considered.
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -161,7 +161,7 @@ private Sequence<Result<TimeseriesResultValue>>
processVectorized(
);
if (granularizer == null) {
- return Sequences.empty();
+ return Sequences.withBaggage(Sequences.empty(), closer);
Review Comment:
nice catch :+1:
##########
core/pom.xml:
##########
@@ -430,12 +430,6 @@
<configuration>
<!-- use normal classpath instead of manifest jar for
JvmUtilsTest.testSystemClassPath -->
<useManifestOnlyJar>false</useManifestOnlyJar>
- <argLine>
- @{jacocoArgLine}
- ${jdk.surefire.argLine}
-
-Djava.library.path=${project.build.directory}/hyperic-sigar-${sigar.base.version}/sigar-bin/lib/
Review Comment:
i guess this line didn't matter? (i don't see it in parent pom)
##########
processing/src/main/java/org/apache/druid/segment/ColumnCache.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment;
+
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.segment.column.BaseColumn;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.apache.druid.segment.column.ColumnIndexSupplier;
+import org.apache.druid.segment.selector.settable.SettableColumnValueSelector;
+
+import javax.annotation.Nullable;
+import java.util.HashMap;
+import java.util.List;
+
+public class ColumnCache implements ColumnSelector
+{
+ private final HashMap<String, ColumnHolder> holderCache;
+ private final QueryableIndex index;
+ private final Closer closer;
+
+ public ColumnCache(QueryableIndex index, Closer closer)
+ {
+ this.index = index;
+ this.closer = closer;
+
+ this.holderCache = new HashMap<>();
+ }
+
+
+ @Override
+ public List<String> getColumnNames()
+ {
+ return index.getColumnNames();
+ }
+
+ @Nullable
+ @Override
+ public ColumnHolder getColumnHolder(String columnName)
+ {
+ return holderCache.computeIfAbsent(columnName, dimension -> {
+ // Here we do a funny little dance to memoize the BaseColumn and
register it with the closer.
+ // It would probably be cleaner if the ColumnHolder itself was
`Closeable` and did its own memoization,
+ // but that change is much wider and runs the risk of even more things
that need to close the thing
+ // not actually closing it. So, maybe this is a hack, maybe it's a wise
decision, who knows, but at
+ // least for now, we grab the holder, grab the column, register the
column with the closer and then return
+ // a new holder that always returns the same reference for the column.
+
+ final ColumnHolder holder = index.getColumnHolder(columnName);
+ if (holder == null) {
+ return null;
+ }
+
+ return new ColumnHolder()
+ {
+ @Nullable
+ private BaseColumn theColumn = null;
+
+ @Override
+ public ColumnCapabilities getCapabilities()
+ {
+ return holder.getCapabilities();
+ }
+
+ @Override
+ public int getLength()
+ {
+ return holder.getLength();
+ }
+
+ @Override
+ public BaseColumn getColumn()
+ {
+ if (theColumn == null) {
+ theColumn = closer.register(holder.getColumn());
+ }
+ return theColumn;
+ }
+
+ @Nullable
+ @Override
+ public ColumnIndexSupplier getIndexSupplier()
+ {
+ return holder.getIndexSupplier();
Review Comment:
Afaik they currently don't have anything to close, but it makes sense to me
to make them closable eventually so that they could if necessary. It could also
possibly be part of the `BaseColumn`, though that might limit or require a
different mechanism if we ever add indexes that are for multiple columns, and
also requires consideration for how virtual columns would close their indexes
since they don't currently close stuff, so maybe is better to just make the
index supplier closable. I'm not sure it needs to be done in this PR though.
--
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]
