You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2024/06/01 08:43:59 UTC
[PR] rework cursor creation (druid)
clintropolis opened a new pull request, #16533:
URL: https://github.com/apache/druid/pull/16533
### Description
_todo: detailed description_
changes:
* Added `CursorBuildSpec` which captures all of the 'interesting' stuff that goes into producing a cursor as a replacement for the method arguments of `CursorFactory.canVectorize`, `CursorFactory.makeCursors`, and `CursorFactory.makeVectorCursors`
* added new interfaces `CursorMaker` and new method `asCursorMaker` to `CursorFactory`, which takes a `CursorBuildSpec` as an argument and replaces `CursorFactory.canVectorize`, `CursorFactory.makeCursors`, and `CursorFactory.makeVectorCursors`
* Deprecated `CursorFactory.canVectorize`, `CursorFactory.makeCursors`, and `CursorFactory.makeVectorCursors`
* updated all `CursorFactory` implementations to implement `asCursorMaker`
* updated all query engines to use `asCursorMaker`
#### Release note
_todo_
<hr>
##### Key changed/added classes in this PR
* `CursorFactory`
* `CursorMaker`
* `CursorBuildSpec`
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704293481
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
I was thinking the use case is that someone wants to do a bunch of `GROUP BY sourceIp` and `GROUP BY destinationIp` queries, and to speed those up we'd want to store two projections, one sorted by `sourceIp` and one by `destinationIp`. The grouping engine could do a streaming aggregation, without any temporary hash table, because the rows are already in the right sorted order. We don't have the code yet in `groupBy` to do this but I was thinking we would add it.
To support that, the query engines would need some way of passing a preferred ordering to the cursor maker. The cursor I think would also need a way of telling the query engine what its sort order actually is.
> Specifically, I'm thinking mostly about topN and groupBy, which never set descending and do their own sorting. Group by has an additional problem, which if we order by time descending, with this new logic it would try to do that to the cursor, which would mean it couldn't vectorize, but the engine doesn't actually care if the results are ordered by the queries order by or not and expects everything to be not descending today.
For this, given the fact that the grouping engines don't take advantage of sortedness today, they should pass null or empty for the preferred sort order parameter. If they do start caring at some point, that would change then.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1623185997
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Sequence<Cursor> makeCursors()
+ {
+ return CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.getGranularity(),
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeCursors](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7432)
##########
processing/src/main/java/org/apache/druid/segment/FilteredStorageAdapter.java:
##########
@@ -44,6 +44,24 @@
this.filterOnDataSource = filter;
}
+ @Override
+ public CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ final CursorBuildSpec.CursorBuildSpecBuilder buildSpecBuilder = CursorBuildSpec.builder(spec);
+ final Filter newFilter;
+ if (spec.getFilter() == null) {
+ if (filterOnDataSource != null) {
+ newFilter = filterOnDataSource.toFilter();
+ } else {
+ newFilter = null;
+ }
+ } else {
+ newFilter = new AndFilter(ImmutableList.of(spec.getFilter(), filterOnDataSource.toFilter()));
Review Comment:
## Dereferenced variable may be null
Variable [filterOnDataSource](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7438)
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Sequence<Cursor> makeCursors()
+ {
+ return CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.getGranularity(),
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ );
+ }
+
+ @Override
+ public VectorCursor makeVectorCursor()
+ {
+ return CursorFactory.this.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.isDescending(),
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeVectorCursor](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7433)
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.canVectorize](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7431)
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorMaker.java:
##########
@@ -668,4 +724,43 @@
return new DescendingTimestampCheckingOffset(baseOffset.clone(), timestamps, timeLimit, allWithinThreshold);
}
}
+
+ private final class CursorResources implements Closeable
Review Comment:
## Inner class could be static
CursorResources could be made static, since the enclosing instance is used only in its constructor.
[Show more details](https://github.com/apache/druid/security/code-scanning/7436)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1717805127
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -94,64 +103,46 @@ public Sequence<Result<TimeseriesResultValue>> process(
);
}
- final Filter filter = Filters.convertToCNFFromQueryContext(query, Filters.toFilter(query.getFilter()));
final Interval interval = Iterables.getOnlyElement(query.getIntervals());
final Granularity gran = query.getGranularity();
- final boolean descending = query.isDescending();
- final ColumnInspector inspector = query.getVirtualColumns().wrapInspector(adapter);
-
- final boolean doVectorize = query.context().getVectorize().shouldVectorize(
- adapter.canVectorize(filter, query.getVirtualColumns(), descending)
- && VirtualColumns.shouldVectorize(query, query.getVirtualColumns(), adapter)
- && VectorGroupByEngine.canVectorizeAggregators(inspector, query.getAggregatorSpecs())
- );
+ final CursorHolder cursorHolder = adapter.makeCursorHolder(makeCursorBuildSpec(query, timeseriesQueryMetrics));
+ Cursors.requireTimeOrdering(cursorHolder, query.isDescending() ? Order.DESCENDING : Order.ASCENDING);
final Sequence<Result<TimeseriesResultValue>> result;
- if (doVectorize) {
- result = processVectorized(query, adapter, filter, interval, gran, descending, timeseriesQueryMetrics);
+ if (query.context().getVectorize().shouldVectorize(cursorHolder.canVectorize(), cursorHolder::close)) {
+ result = processVectorized(query, adapter, cursorHolder, interval, gran);
Review Comment:
good call, i meant to do this anyway to dump the funny shouldVectorize thing
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQueryEngine.java:
##########
@@ -193,13 +205,74 @@ private static boolean canUsePooledAlgorithm(
// non-string output cannot use the pooled algorith, even if the underlying selector supports it
return false;
}
- if (Types.is(capabilities, ValueType.STRING)) {
- // string columns must use the on heap algorithm unless they have the following capabilites
- return capabilities.isDictionaryEncoded().isTrue() && capabilities.areDictionaryValuesUnique().isTrue();
- } else {
+ if (!Types.is(capabilities, ValueType.STRING)) {
// non-strings are not eligible to use the pooled algorithm, and should use a heap algorithm
return false;
}
+
+ // string columns must use the on heap algorithm unless they have the following capabilites
+ if (!capabilities.isDictionaryEncoded().isTrue() || !capabilities.areDictionaryValuesUnique().isTrue()) {
+ return false;
+ }
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ // all other requirements have been satisfied, ALL granularity can always use the pooled algorithms
+ return true;
+ }
+ // if not using ALL granularity, we can still potentially use the pooled algorithm if we are certain it doesn't
+ // need to make multiple passes (e.g. reset the cursor)
+ try (final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take()) {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
Review Comment:
cool, i was just copying stuff from https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/topn/PooledTopNAlgorithm.java#L259 and didn't think too much about it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702310851
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
Review Comment:
this is definitely here to support projections since they are currently planned to be always rollup based, so we need to be able to find the coarsest granularity that matches the request. I think could probably match virtual columns, but we are already passing in virtual columns too and try to match that up with any granularity-like virtual columns. I think we would still need to pass in query granularity though in case it was specified on a native query 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259448589
TimeseriesBenchmark.queryMultiQueryableIndex
master:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.A avgt 5 76084.860 ± 5899.304 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 8264.808 ± 240.601 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 67659.434 ± 842.718 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 46.947 ± 0.880 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 85119.693 ± 901.179 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 15222.717 ± 172.699 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 97082.562 ± 1261.596 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 46.379 ± 1.104 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.A avgt 5 41654.123 ± 1637.797 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 2544.556 ± 20.861 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 64024.533 ± 1514.812 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 47.612 ± 1.526 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 45452.703 ± 1172.880 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 2630.211 ± 40.912 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 90311.756 ± 3310.283 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 46.457 ± 1.187 us/op
```
PR:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.A avgt 5 71144.946 ± 654.946 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 9756.647 ± 94.576 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 66541.959 ± 399.063 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 45.455 ± 0.605 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 78600.110 ± 1325.356 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 12283.960 ± 176.547 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 69956.843 ± 341.464 us/op
TimeseriesBenchmark.queryMultiQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 44.630 ± 1.135 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.A avgt 5 40028.528 ± 435.349 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 2500.597 ± 21.100 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 61864.467 ± 2030.859 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 45.444 ± 0.543 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 43840.173 ± 148.887 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 2529.889 ± 29.915 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 63574.108 ± 804.186 us/op
TimeseriesBenchmark.queryMultiQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 44.704 ± 0.508 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1626486865
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> columns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> columns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.columns = columns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getColumns()
+ {
+ return columns;
+ }
+
Review Comment:
thanks for having a look! i actually plan to use this in a follow-up PR but i haven't had a chance to fill out the PR description yet to clear that up, i'll work on that soon. So for now i'll probably just mark this to be ignored or write some tests for the query to buildspec conversion logic so that it actually gets used.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1627196668
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
Review Comment:
this is intended so that existing `CursorFactory`/`StorageAdapter` implementations can keep working with query engines without implementing `asCursorMaker`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1693726787
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/StorageAdapterRowsAndColumns.java:
##########
@@ -98,45 +95,36 @@
@Nonnull
private static RowsAndColumns materialize(StorageAdapter as)
{
- final Sequence<Cursor> cursors = as.makeCursors(
- null,
- Intervals.ETERNITY,
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
-
- RowSignature rowSignature = as.getRowSignature();
-
- FrameWriter writer = cursors.accumulate(null, (accumulated, in) -> {
- if (accumulated != null) {
- // We should not get multiple cursors because we set the granularity to ALL. So, this should never
- // actually happen, but it doesn't hurt us to defensive here, so we test against it.
- throw new ISE("accumulated[%s] non-null, why did we get multiple cursors?", accumulated);
+ try (final CursorMaker maker = as.asCursorMaker(CursorBuildSpec.FULL_SCAN)) {
+ final Cursor cursor = maker.makeCursor();
+
+ if (cursor == null) {
+ return new EmptyRowsAndColumns();
}
+
+ final RowSignature rowSignature = as.getRowSignature();
- final ColumnSelectorFactory columnSelectorFactory = in.getColumnSelectorFactory();
+ final ColumnSelectorFactory columnSelectorFactory = cursor.getColumnSelectorFactory();
final FrameWriterFactory frameWriterFactory = FrameWriters.makeColumnBasedFrameWriterFactory(
new ArenaMemoryAllocatorFactory(200 << 20), // 200 MB, because, why not?
rowSignature,
Collections.emptyList()
);
- final FrameWriter frameWriter = frameWriterFactory.newFrameWriter(columnSelectorFactory);
- while (!in.isDoneOrInterrupted()) {
- frameWriter.addSelection();
- in.advance();
+ try(final FrameWriter writer = frameWriterFactory.newFrameWriter(columnSelectorFactory)) {
+ while (!cursor.isDoneOrInterrupted()) {
+ writer.addSelection();
Review Comment:
## Dereferenced variable may be null
Variable [writer](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7620)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1717804066
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -154,10 +165,26 @@ public boolean shouldVectorize(final boolean canVectorize)
return true;
}
+
+ @Override
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
+ {
+ if (!canVectorize) {
+ cleanup.run();
+ throw new ISE("Cannot vectorize!");
+ }
+
+ return true;
+ }
};
public abstract boolean shouldVectorize(boolean canVectorize);
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
Review Comment:
ah yea, this has a `Runnable` from before `CursorHolder` was `Closeable`, which was before dumped the `Sequence<Cursor>` stuff. I actually meant to rework things to not need this at all
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702429974
##########
processing/src/main/java/org/apache/druid/query/vector/VectorCursorGranularizer.java:
##########
@@ -102,6 +71,37 @@ public static VectorCursorGranularizer create(
return new VectorCursorGranularizer(cursor, bucketIterable, timeSelector);
}
+ // And a cursor that has been made from it.
Review Comment:
removed
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704660491
##########
processing/src/main/java/org/apache/druid/segment/StorageAdapter.java:
##########
@@ -33,8 +35,66 @@
/**
*/
@PublicApi
-public interface StorageAdapter extends CursorFactory, ColumnInspector
+public interface StorageAdapter extends CursorFactory, ColumnInspector, CursorMakerFactory
{
+
+ /**
+ * Build a {@link CursorHolder} which can provide {@link Cursor} and {@link VectorCursor} (if capable) which allows
+ * scanning segments and creating {@link ColumnSelectorFactory} and
+ * {@link org.apache.druid.segment.vector.VectorColumnSelectorFactory} respectively to read row values at the cursor
+ * position.
+ */
+ @Override
+ default CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return StorageAdapter.this.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ CursorBuildSpec.preferDescendingTimeOrder(spec.getPreferredOrdering())
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.canVectorize](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7669)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259459518
TimeseriesBenchmark.querySingleQueryableIndex
master:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.A avgt 5 77734.500 ± 902.113 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 8198.883 ± 216.497 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 66904.732 ± 735.733 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 33.132 ± 0.381 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 82145.092 ± 1500.217 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 15125.089 ± 183.400 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 72453.139 ± 2111.685 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.846 ± 1.069 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.A avgt 5 40533.929 ± 400.952 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 2545.088 ± 74.228 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 64272.494 ± 2781.256 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 34.397 ± 1.504 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 44768.343 ± 923.471 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 2616.776 ± 189.977 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 65934.521 ± 1614.295 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 33.168 ± 0.344 us/op
```
PR:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.A avgt 5 72130.764 ± 438.658 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 9788.415 ± 153.642 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 66024.324 ± 1004.626 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 32.723 ± 2.410 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 78580.605 ± 958.057 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 12245.488 ± 205.838 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 69773.751 ± 244.531 us/op
TimeseriesBenchmark.querySingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.043 ± 0.308 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.A avgt 5 39699.051 ± 554.411 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 2475.190 ± 10.130 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 61482.618 ± 670.436 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 32.674 ± 0.559 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 43351.282 ± 406.655 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 2518.789 ± 29.298 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 63343.235 ± 508.815 us/op
TimeseriesBenchmark.querySingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.095 ± 0.446 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710268469
##########
processing/src/main/java/org/apache/druid/segment/RowBasedCursor.java:
##########
@@ -140,4 +148,12 @@ private void advanceToMatchingRow()
rowId++;
}
}
+
+ private void advanceToMatchingMarkRow()
+ {
+ while (!isDone() && rowId < markId) {
Review Comment:
This should be checking `valueMatcher` somehow, right? I think as written, it will advance `markId` number of rows on the underlying `rowWalker`, without regard for which ones match the provided filter. Is that ok?
Additionally, this logic creates a requirement that `rowWalker` be immutable. I don't think that'll always be true. For example, we use this cursor on top of external data, where re-walking the walker means re-fetching a file. The file could have changed. We also use it on top of lookups, where re-walking could get a different set of keys. I am not sure how really to do sensible marking on data streams like this.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259041296
TopNBenchmark (modified so granularity is a parameter)
master:
```
Benchmark (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) (threshold) Mode Cnt Score Error Units
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.A 10 avgt 5 105630.245 ± 2072.370 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.numericSort 10 avgt 5 83544.713 ± 9846.901 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.alphanumericSort 10 avgt 5 85465.242 ± 2333.781 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.A 10 avgt 5 167081.595 ± 1660.539 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.numericSort 10 avgt 5 458993.674 ± 25923.338 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.alphanumericSort 10 avgt 5 491948.503 ± 22870.460 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.A 10 avgt 5 2052523.983 ± 24549.726 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.numericSort 10 avgt 5 178007.256 ± 3952.668 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.alphanumericSort 10 avgt 5 186421.930 ± 2880.129 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.A 10 avgt 5 2061836.038 ± 20842.268 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.numericSort 10 avgt 5 326769.712 ± 4152.047 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.alphanumericSort 10 avgt 5 368820.273 ± 1842.130 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.A 10 avgt 5 106479.837 ± 6583.920 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.numericSort 10 avgt 5 39089.718 ± 2045.293 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.alphanumericSort 10 avgt 5 48079.409 ± 1045.799 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.A 10 avgt 5 107774.370 ± 1272.252 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.numericSort 10 avgt 5 216689.632 ± 3060.145 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.alphanumericSort 10 avgt 5 228247.136 ± 2471.377 us/op
```
PR:
```
Benchmark (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) (threshold) Mode Cnt Score Error Units
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.A 10 avgt 5 100383.953 ± 967.799 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.numericSort 10 avgt 5 82141.412 ± 986.582 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 all 750000 basic.alphanumericSort 10 avgt 5 85318.596 ± 445.816 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.A 10 avgt 5 164062.058 ± 2860.871 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.numericSort 10 avgt 5 464774.711 ± 5786.066 us/op
TopNBenchmark.queryMultiQueryableIndex N/A 1 hour 750000 basic.alphanumericSort 10 avgt 5 484269.075 ± 4745.483 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.A 10 avgt 5 2056522.120 ± 30632.561 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.numericSort 10 avgt 5 174853.221 ± 4331.467 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A all 750000 basic.alphanumericSort 10 avgt 5 188507.677 ± 14648.549 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.A 10 avgt 5 2084291.490 ± 8153.473 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.numericSort 10 avgt 5 346339.898 ± 6900.969 us/op
TopNBenchmark.querySingleIncrementalIndex onheap N/A hour 750000 basic.alphanumericSort 10 avgt 5 392365.185 ± 3409.220 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.A 10 avgt 5 99605.700 ± 602.251 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.numericSort 10 avgt 5 40322.660 ± 1029.302 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 all 750000 basic.alphanumericSort 10 avgt 5 49522.946 ± 333.189 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.A 10 avgt 5 109509.117 ± 826.205 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.numericSort 10 avgt 5 199704.033 ± 13133.948 us/op
TopNBenchmark.querySingleQueryableIndex N/A 1 hour 750000 basic.alphanumericSort 10 avgt 5 228998.883 ± 2610.124 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "asdf2014 (via GitHub)" <gi...@apache.org>.
asdf2014 commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1625476842
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> columns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> columns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.columns = columns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getColumns()
+ {
+ return columns;
+ }
+
Review Comment:
```suggestion
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1627197171
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Sequence<Cursor> makeCursors()
+ {
+ return CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.getGranularity(),
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ );
Review Comment:
this is intended so that existing `CursorFactory`/`StorageAdapter` implementations can keep working with query engines without implementing `asCursorMaker`
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -35,12 +38,55 @@
*/
public interface CursorFactory
{
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Sequence<Cursor> makeCursors()
+ {
+ return CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.getGranularity(),
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ );
+ }
+
+ @Override
+ public VectorCursor makeVectorCursor()
+ {
+ return CursorFactory.this.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.isDescending(),
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
Review Comment:
this is intended so that existing `CursorFactory`/`StorageAdapter` implementations can keep working with query engines without implementing `asCursorMaker`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710314520
##########
processing/src/test/java/org/apache/druid/frame/segment/FrameStorageAdapterTest.java:
##########
@@ -345,58 +367,46 @@ public void tearDown()
}
@Test
- public void test_makeCursors()
+ public void test_makeCursor()
{
- assertCursorsMatch(
- adapter ->
- adapter.makeCursors(
- filter,
- interval,
- virtualColumns,
- Granularities.ALL, // Frames only support Granularities.ALL: no point testing the others.
- descending,
- null
- )
- );
+ assertCursorMatch(adapter -> adapter.makeCursorHolder(buildSpec));
}
@Test
public void test_makeVectorCursor()
{
- Assume.assumeTrue(frameAdapter.canVectorize(filter, virtualColumns, descending));
-
- assertVectorCursorsMatch(
- adapter ->
- adapter.makeVectorCursor(
- filter,
- interval,
- virtualColumns,
- descending,
- VECTOR_SIZE,
- null
- )
- );
+ assertVectorCursorsMatch(adapter -> adapter.makeCursorHolder(buildSpec));
}
- private void assertCursorsMatch(final Function<StorageAdapter, Sequence<Cursor>> call)
+ private void assertCursorMatch(final Function<StorageAdapter, CursorHolder> call)
{
final RowSignature signature = frameAdapter.getRowSignature();
- final Sequence<List<Object>> queryableRows =
- call.apply(queryableAdapter).flatMap(cursor -> FrameTestUtil.readRowsFromCursor(cursor, signature));
- final Sequence<List<Object>> frameRows =
- call.apply(frameAdapter)
- .flatMap(cursor -> FrameTestUtil.readRowsFromCursor(advanceAndReset(cursor), signature));
- FrameTestUtil.assertRowsEqual(queryableRows, frameRows);
+ try (final CursorHolder queryableMaker = call.apply(queryableAdapter);
+ final CursorHolder frameMaker = call.apply(frameAdapter)) {
+ final Sequence<List<Object>> queryableRows =
+ FrameTestUtil.readRowsFromCursor(queryableMaker.asCursor(), signature);
+ final Sequence<List<Object>> frameRows =
+ FrameTestUtil.readRowsFromCursor(frameMaker.asCursor(), signature);
Review Comment:
oops, this was an oversight, will fix
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702431018
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -271,4 +275,24 @@ default Set<String> getRequiredColumns()
{
return null;
}
+
+ default CursorBuildSpec asCursorBuildSpec(@Nullable QueryMetrics<?> queryMetrics)
+ {
+ final List<Interval> intervals = getIntervals();
+ if (intervals.size() > 1) {
Review Comment:
cool, i didn't know about this method, switched to use it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1718036509
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -94,64 +103,46 @@ public Sequence<Result<TimeseriesResultValue>> process(
);
}
- final Filter filter = Filters.convertToCNFFromQueryContext(query, Filters.toFilter(query.getFilter()));
final Interval interval = Iterables.getOnlyElement(query.getIntervals());
final Granularity gran = query.getGranularity();
- final boolean descending = query.isDescending();
- final ColumnInspector inspector = query.getVirtualColumns().wrapInspector(adapter);
-
- final boolean doVectorize = query.context().getVectorize().shouldVectorize(
- adapter.canVectorize(filter, query.getVirtualColumns(), descending)
- && VirtualColumns.shouldVectorize(query, query.getVirtualColumns(), adapter)
- && VectorGroupByEngine.canVectorizeAggregators(inspector, query.getAggregatorSpecs())
- );
+ final CursorHolder cursorHolder = adapter.makeCursorHolder(makeCursorBuildSpec(query, timeseriesQueryMetrics));
+ Cursors.requireTimeOrdering(cursorHolder, query.isDescending() ? Order.DESCENDING : Order.ASCENDING);
final Sequence<Result<TimeseriesResultValue>> result;
- if (doVectorize) {
- result = processVectorized(query, adapter, filter, interval, gran, descending, timeseriesQueryMetrics);
+ if (query.context().getVectorize().shouldVectorize(cursorHolder.canVectorize(), cursorHolder::close)) {
+ result = processVectorized(query, adapter, cursorHolder, interval, gran);
Review Comment:
other callers looked good to me
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1715828772
##########
processing/src/main/java/org/apache/druid/segment/FilteredStorageAdapter.java:
##########
@@ -45,26 +42,21 @@ public FilteredStorageAdapter(final StorageAdapter adapter, final DimFilter filt
}
@Override
- public Sequence<Cursor> makeCursors(
- @Nullable Filter filter,
- Interval interval,
- VirtualColumns virtualColumns,
- Granularity gran,
- boolean descending,
- @Nullable QueryMetrics<?> queryMetrics
- )
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
{
- final Filter andFilter;
- if (filter == null) {
+ final CursorBuildSpec.CursorBuildSpecBuilder buildSpecBuilder = CursorBuildSpec.builder(spec);
+ final Filter newFilter;
+ if (spec.getFilter() == null) {
if (filterOnDataSource != null) {
Review Comment:
`FilteredDataSource` allows null for some reason, so i just marked it nullable and added a check for now. I don't really understand why we would allow filter to be null though, that doesn't seem very useful, so it might be worth looking into changing it to ensure not null in 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259448027
TimeseriesBenchmark.queryFilteredSingleQueryableIndex
master:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.A avgt 5 6614.254 ± 136.123 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 1944.037 ± 10.366 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 2019.249 ± 95.584 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 33.451 ± 0.451 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 11040.082 ± 180.823 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 4274.352 ± 79.384 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 4309.323 ± 30.720 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 33.200 ± 0.739 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.A avgt 5 6470.706 ± 53.526 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 1961.523 ± 367.082 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 1987.598 ± 60.148 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 33.445 ± 0.516 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 8405.975 ± 143.986 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 1940.375 ± 33.468 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 1984.012 ± 15.155 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.945 ± 0.523 us/op
```
PR:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.A avgt 5 6610.128 ± 92.993 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterNumeric avgt 5 1920.479 ± 18.626 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 1973.976 ± 7.029 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 all 750000 basic.timeFilterByInterval avgt 5 32.789 ± 0.478 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.A avgt 5 8630.230 ± 105.645 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 1933.904 ± 11.490 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 1986.944 ± 22.651 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex true N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.511 ± 1.069 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.A avgt 5 6325.689 ± 42.639 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterNumeric avgt 5 1913.886 ± 19.559 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterAlphanumeric avgt 5 1966.111 ± 58.022 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 all 750000 basic.timeFilterByInterval avgt 5 32.788 ± 0.528 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.A avgt 5 8263.713 ± 51.142 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterNumeric avgt 5 1923.849 ± 11.076 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterAlphanumeric avgt 5 1977.200 ± 36.164 us/op
TimeseriesBenchmark.queryFilteredSingleQueryableIndex false N/A 1 hour 750000 basic.timeFilterByInterval avgt 5 32.330 ± 0.268 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1715835436
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinEngine.java:
##########
@@ -250,12 +244,32 @@ public boolean isDoneOrInterrupted()
return isDone() || Thread.currentThread().isInterrupted();
}
+ @Override
+ public void mark()
+ {
+ joinMarkId = joinColumnSelectorFactory.getRowId();
+ }
+
+ @Override
+ public void resetToMark()
+ {
+ leftCursor.reset();
+ joinMatcher.reset();
+ joinColumnSelectorFactory.resetRowId();
+ initialize();
+ while (!isDone() && joinColumnSelectorFactory.getRowId() < joinMarkId) {
Review Comment:
discussed offline but to update here, for now have decided to update these and loosen the contract so that mark/resetToMark only guarantee return to same timestamp since that is all topN needs, and also since stuff like lookup segments could change on iterating again, and added some tests about it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1717807468
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQueryEngine.java:
##########
@@ -193,13 +205,74 @@ private static boolean canUsePooledAlgorithm(
// non-string output cannot use the pooled algorith, even if the underlying selector supports it
return false;
}
- if (Types.is(capabilities, ValueType.STRING)) {
- // string columns must use the on heap algorithm unless they have the following capabilites
- return capabilities.isDictionaryEncoded().isTrue() && capabilities.areDictionaryValuesUnique().isTrue();
- } else {
+ if (!Types.is(capabilities, ValueType.STRING)) {
// non-strings are not eligible to use the pooled algorithm, and should use a heap algorithm
return false;
}
+
+ // string columns must use the on heap algorithm unless they have the following capabilites
+ if (!capabilities.isDictionaryEncoded().isTrue() || !capabilities.areDictionaryValuesUnique().isTrue()) {
+ return false;
+ }
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ // all other requirements have been satisfied, ALL granularity can always use the pooled algorithms
+ return true;
+ }
+ // if not using ALL granularity, we can still potentially use the pooled algorithm if we are certain it doesn't
+ // need to make multiple passes (e.g. reset the cursor)
+ try (final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take()) {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
+ final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
+
+ return numValuesPerPass <= cardinality;
+ }
+ }
+
+ private static boolean shouldUseAggregateMetricFirstAlgorithm(TopNQuery query, TopNAlgorithmSelector selector)
+ {
+ // must be using ALL granularity since it makes multiple passes and must reset the cursor
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ return selector.isAggregateTopNMetricFirst() || query.context().getBoolean("doAggregateTopNMetricFirst", false);
+ }
+ return false;
+ }
+
+ public static CursorBuildSpec makeCursorBuildSpec(TopNQuery query, @Nullable QueryMetrics<?> queryMetrics)
+ {
+ // virtual column is currently only used as a decorator to pass to the cursor holder to allow specializing cursor
+ // and vector cursors if any pre-aggregated data at the matching granularity is available
+ // eventually this could probably be reworked to be used by the granularizer instead of the existing method
+ // of creating a selector on the time column
+ final VirtualColumn granularityVirtual = Granularities.toVirtualColumn(query);
Review Comment:
This was originally the funny method on `CursorBuildSpec` that took grouping columns, virtual columns, and granularity, but that was a bit too strange. Maybe it should be a static method on `Granularities` that accepts a `Query` and a `CursorBuildSpecBuilder`, will play around with it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704301823
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
+ */
boolean isDone();
+
+ /**
+ * Check if there are any additional rows in the cursor, or if the thread has been interrupted
+ */
boolean isDoneOrInterrupted();
+
+ /**
+ * Mark a position on the cursor which can recalled with {@link #resetToMark()}
+ */
+ void mark(DateTime mark);
Review Comment:
I think it could still use more clarification. If I'm writing a query engine that isn't topN, and thinking that I might want to use the `mark(DateTime)` method, this javadoc doesn't tell me enough about what it does.
Specifically I wonder: when a cursor implementation decides to use the `DateTime mark` instead of the current offset, what behavior can I expect? Will `resetToMark()` reset to the first row with that timestamp? Any row with that timestamp? What if no rows have that particular timestamp-- will it reset to the first row with a timestamp greater than the marked timestamp? Are there any situations where behavior is undefined or where I'd get an error?
It also feels useful to call out what the expected value for `DateTime mark` is. Is it expected to match the `__time` value of the current row?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710310631
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -32,35 +33,59 @@
* Interface extended by {@link StorageAdapter}, which gives them the power to create cursors.
*
* @see StorageAdapter
+ *
+ * @deprecated This interface is deprecated and no longer implemented by any built-in {@link StorageAdapter}. Callers
+ * should use {@link CursorMakerFactory#makeCursorHolder(CursorBuildSpec)} instead. Implementors should implement
+ * {@link CursorMakerFactory#makeCursorHolder(CursorBuildSpec)} instead.
*/
+@Deprecated
Review Comment:
Oops, I didn't mean to actually leave this comment. I changed my mind, I think it's ok to shift to the new interface now as long as we keep certain parts of it that may evolve (such as `setGroupingAndVirtualColumns`) marked specifically as should-be-avoided-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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1703791887
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
still working on this one, I have it working using the scan orderBy classes as a `List` of ordering columns, but i'm a bit unsure about what the appropriate thing to do in all cases is.
Specifically, I'm thinking mostly about topN and groupBy, which never set descending and do their own sorting. Group by has an additional problem, which if we order by time descending, with this new logic it would try to do that to the cursor, which would mean it couldn't vectorize, but the engine doesn't actually care if the results are ordered by the queries order by or not and expects everything to be not descending today. And even if they were already ordered from a projection or something, it seems like right now no way for the engines to take advantage of this, so we probably want a method on the cursor perhaps to expose how the cursor itself is ordered?
The workaround I have right now is basically only set these order by columns on the cursor build spec for scan when ordered by time, timeseries, and timeboundary queries, everything else sets it to null. What should we do here?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1703783700
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
Review Comment:
this is done, seems nicer
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710297048
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.scan.Order;
+import org.apache.druid.query.scan.OrderBy;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+ @Nullable
+ private final List<OrderBy> orderByColumns;
+
+ private final QueryContext queryContext;
+
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ @Nullable List<OrderBy> preferredOrdering,
+ QueryContext queryContext,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.orderByColumns = preferredOrdering;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ @Nullable
+ public List<OrderBy> getPreferredOrdering()
+ {
+ return orderByColumns;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static boolean preferDescendingTimeOrder(@Nullable List<OrderBy> preferredOrdering)
+ {
+ if (preferredOrdering != null && preferredOrdering.size() == 1) {
+ final OrderBy orderBy = Iterables.getOnlyElement(preferredOrdering);
+ return ColumnHolder.TIME_COLUMN_NAME.equals(orderBy.getColumnName()) && Order.DESCENDING == orderBy.getOrder();
+ }
+ return false;
+ }
+
+ public static class CursorBuildSpecBuilder
+ {
+ @Nullable
+ private Filter filter;
+ private Interval interval = Intervals.ETERNITY;
+
+ @Nullable
+ private List<String> groupingColumns;
+ private VirtualColumns virtualColumns = VirtualColumns.EMPTY;
+ @Nullable
+ private List<AggregatorFactory> aggregators;
+ @Nullable
+ private List<OrderBy> preferredOrdering;
+
+ private QueryContext queryContext = QueryContext.empty();
+ @Nullable
+ private QueryMetrics<?> queryMetrics;
+
+ private CursorBuildSpecBuilder()
+ {
+ // initialize with defaults
+ }
+
+ private CursorBuildSpecBuilder(CursorBuildSpec buildSpec)
+ {
+ this.filter = buildSpec.filter;
+ this.interval = buildSpec.interval;
+ this.groupingColumns = buildSpec.groupingColumns;
+ this.virtualColumns = buildSpec.virtualColumns;
+ this.aggregators = buildSpec.aggregators;
+ this.preferredOrdering = buildSpec.orderByColumns;
+ this.queryContext = buildSpec.queryContext;
+ this.queryMetrics = buildSpec.queryMetrics;
+ }
+
+ public CursorBuildSpecBuilder setFilter(@Nullable Filter filter)
+ {
+ this.filter = filter;
+ return this;
+ }
+
+ public CursorBuildSpecBuilder setInterval(Interval interval)
+ {
+ this.interval = interval;
+ return this;
+ }
+
+ public CursorBuildSpecBuilder setGroupingAndVirtualColumns(
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns
+ )
+ {
+ final VirtualColumn granularityVirtual = Granularities.toVirtualColumn(granularity);
Review Comment:
yea, i guess doing the query -> buildspec translation in the engines would make more sense which i think also would satisfy many of your complaints, will move it there.
as for the other thing, maybe we should make a specialized time floor virtual column to use instead of expression version, will think about it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1717639536
##########
processing/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java:
##########
@@ -68,6 +73,11 @@ public static ExprMacroTable nil()
return NIL;
}
+ public static ExprMacroTable granularity()
Review Comment:
Could use some javadoc to explain the purpose of this method. I don't think it would be obvious to someone that isn't very familiar with how query engines handle granularity and how that maps onto projections.
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQueryEngine.java:
##########
@@ -193,13 +205,74 @@ private static boolean canUsePooledAlgorithm(
// non-string output cannot use the pooled algorith, even if the underlying selector supports it
return false;
}
- if (Types.is(capabilities, ValueType.STRING)) {
- // string columns must use the on heap algorithm unless they have the following capabilites
- return capabilities.isDictionaryEncoded().isTrue() && capabilities.areDictionaryValuesUnique().isTrue();
- } else {
+ if (!Types.is(capabilities, ValueType.STRING)) {
// non-strings are not eligible to use the pooled algorithm, and should use a heap algorithm
return false;
}
+
+ // string columns must use the on heap algorithm unless they have the following capabilites
+ if (!capabilities.isDictionaryEncoded().isTrue() || !capabilities.areDictionaryValuesUnique().isTrue()) {
+ return false;
+ }
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ // all other requirements have been satisfied, ALL granularity can always use the pooled algorithms
+ return true;
+ }
+ // if not using ALL granularity, we can still potentially use the pooled algorithm if we are certain it doesn't
+ // need to make multiple passes (e.g. reset the cursor)
+ try (final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take()) {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
Review Comment:
Could also check `resultsBuf.capacity()` without doing `resultsBuf.clear()`.
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQueryEngine.java:
##########
@@ -193,13 +205,74 @@ private static boolean canUsePooledAlgorithm(
// non-string output cannot use the pooled algorith, even if the underlying selector supports it
return false;
}
- if (Types.is(capabilities, ValueType.STRING)) {
- // string columns must use the on heap algorithm unless they have the following capabilites
- return capabilities.isDictionaryEncoded().isTrue() && capabilities.areDictionaryValuesUnique().isTrue();
- } else {
+ if (!Types.is(capabilities, ValueType.STRING)) {
// non-strings are not eligible to use the pooled algorithm, and should use a heap algorithm
return false;
}
+
+ // string columns must use the on heap algorithm unless they have the following capabilites
+ if (!capabilities.isDictionaryEncoded().isTrue() || !capabilities.areDictionaryValuesUnique().isTrue()) {
+ return false;
+ }
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ // all other requirements have been satisfied, ALL granularity can always use the pooled algorithms
+ return true;
+ }
+ // if not using ALL granularity, we can still potentially use the pooled algorithm if we are certain it doesn't
+ // need to make multiple passes (e.g. reset the cursor)
+ try (final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take()) {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
+ final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
+
+ return numValuesPerPass <= cardinality;
+ }
+ }
+
+ private static boolean shouldUseAggregateMetricFirstAlgorithm(TopNQuery query, TopNAlgorithmSelector selector)
+ {
+ // must be using ALL granularity since it makes multiple passes and must reset the cursor
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ return selector.isAggregateTopNMetricFirst() || query.context().getBoolean("doAggregateTopNMetricFirst", false);
+ }
+ return false;
+ }
+
+ public static CursorBuildSpec makeCursorBuildSpec(TopNQuery query, @Nullable QueryMetrics<?> queryMetrics)
+ {
+ // virtual column is currently only used as a decorator to pass to the cursor holder to allow specializing cursor
+ // and vector cursors if any pre-aggregated data at the matching granularity is available
+ // eventually this could probably be reworked to be used by the granularizer instead of the existing method
+ // of creating a selector on the time column
+ final VirtualColumn granularityVirtual = Granularities.toVirtualColumn(query);
Review Comment:
Is it possible to make a helper method for this logic? Seems like there's very similar logic in the topN, timeseries, and groupBy engines. Especially if it's planned to be reworked, it's good for the logic to be more centralized so it can be edited & tested in one place.
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -94,64 +103,46 @@ public Sequence<Result<TimeseriesResultValue>> process(
);
}
- final Filter filter = Filters.convertToCNFFromQueryContext(query, Filters.toFilter(query.getFilter()));
final Interval interval = Iterables.getOnlyElement(query.getIntervals());
final Granularity gran = query.getGranularity();
- final boolean descending = query.isDescending();
- final ColumnInspector inspector = query.getVirtualColumns().wrapInspector(adapter);
-
- final boolean doVectorize = query.context().getVectorize().shouldVectorize(
- adapter.canVectorize(filter, query.getVirtualColumns(), descending)
- && VirtualColumns.shouldVectorize(query, query.getVirtualColumns(), adapter)
- && VectorGroupByEngine.canVectorizeAggregators(inspector, query.getAggregatorSpecs())
- );
+ final CursorHolder cursorHolder = adapter.makeCursorHolder(makeCursorBuildSpec(query, timeseriesQueryMetrics));
+ Cursors.requireTimeOrdering(cursorHolder, query.isDescending() ? Order.DESCENDING : Order.ASCENDING);
final Sequence<Result<TimeseriesResultValue>> result;
- if (doVectorize) {
- result = processVectorized(query, adapter, filter, interval, gran, descending, timeseriesQueryMetrics);
+ if (query.context().getVectorize().shouldVectorize(cursorHolder.canVectorize(), cursorHolder::close)) {
+ result = processVectorized(query, adapter, cursorHolder, interval, gran);
Review Comment:
`cursorHolder` can leak here if `processVectorized` throws an error; for example if the "Not enough space for aggregators" case gets hit. Perhaps it makes sense to wrap this entire block (the entire `if` / `else` including `shouldVectorize`) in a `try` that closes the `cursorHolder` if a `Throwable` is caught. If we do that, then I think we don't need the new 2-arg `shouldVectorize` anymore.
Please double check the other query engines for similar leaks.
##########
processing/src/main/java/org/apache/druid/query/topn/AggregateTopNMetricFirstAlgorithm.java:
##########
@@ -112,6 +112,8 @@ public void run(
PooledTopNAlgorithm allMetricAlgo = new PooledTopNAlgorithm(storageAdapter, query, bufferPool);
PooledTopNAlgorithm.PooledTopNParams allMetricsParam = null;
try {
+ // reset cursor to starting position (mark was set by the call to run)
Review Comment:
There's no mark anymore.
##########
processing/src/main/java/org/apache/druid/query/BaseQuery.java:
##########
@@ -66,11 +66,10 @@ public static void checkInterrupted()
public BaseQuery(
DataSource dataSource,
QuerySegmentSpec querySegmentSpec,
- boolean descending,
Map<String, Object> context
)
{
- this(dataSource, querySegmentSpec, descending, context, Granularities.ALL);
+ this(dataSource, querySegmentSpec, false, context, Granularities.ALL);
Review Comment:
Is `isDescending()` needed on the Query interface anymore? (If it ever was?) Since we're doing some breaking changes here for extension-query authors anyway, this seems like a good opportunity to remove it.
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -154,10 +165,26 @@ public boolean shouldVectorize(final boolean canVectorize)
return true;
}
+
+ @Override
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
+ {
+ if (!canVectorize) {
+ cleanup.run();
+ throw new ISE("Cannot vectorize!");
+ }
+
+ return true;
+ }
};
public abstract boolean shouldVectorize(boolean canVectorize);
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
Review Comment:
About the new `cleanup` parameter:
1. Javadoc should explain when it will be called
2. Consider making it `Closeable` instead of `Runnable`; that seems more semantically accurate & it looks like the only time it's actually used, it's with a Closeable thing anyway.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1717803142
##########
processing/src/main/java/org/apache/druid/query/BaseQuery.java:
##########
@@ -66,11 +66,10 @@ public static void checkInterrupted()
public BaseQuery(
DataSource dataSource,
QuerySegmentSpec querySegmentSpec,
- boolean descending,
Map<String, Object> context
)
{
- this(dataSource, querySegmentSpec, descending, context, Granularities.ALL);
+ this(dataSource, querySegmentSpec, false, context, Granularities.ALL);
Review Comment:
yea I don't think it is, only timeseries uses it, I was thinking of doing this as a follow-up, but could do it in this PR too
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1718039213
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQueryEngine.java:
##########
@@ -193,13 +205,74 @@ private static boolean canUsePooledAlgorithm(
// non-string output cannot use the pooled algorith, even if the underlying selector supports it
return false;
}
- if (Types.is(capabilities, ValueType.STRING)) {
- // string columns must use the on heap algorithm unless they have the following capabilites
- return capabilities.isDictionaryEncoded().isTrue() && capabilities.areDictionaryValuesUnique().isTrue();
- } else {
+ if (!Types.is(capabilities, ValueType.STRING)) {
// non-strings are not eligible to use the pooled algorithm, and should use a heap algorithm
return false;
}
+
+ // string columns must use the on heap algorithm unless they have the following capabilites
+ if (!capabilities.isDictionaryEncoded().isTrue() || !capabilities.areDictionaryValuesUnique().isTrue()) {
+ return false;
+ }
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ // all other requirements have been satisfied, ALL granularity can always use the pooled algorithms
+ return true;
+ }
+ // if not using ALL granularity, we can still potentially use the pooled algorithm if we are certain it doesn't
+ // need to make multiple passes (e.g. reset the cursor)
+ try (final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take()) {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
+ final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
+
+ return numValuesPerPass <= cardinality;
+ }
+ }
+
+ private static boolean shouldUseAggregateMetricFirstAlgorithm(TopNQuery query, TopNAlgorithmSelector selector)
+ {
+ // must be using ALL granularity since it makes multiple passes and must reset the cursor
+ if (Granularities.ALL.equals(query.getGranularity())) {
+ return selector.isAggregateTopNMetricFirst() || query.context().getBoolean("doAggregateTopNMetricFirst", false);
+ }
+ return false;
+ }
+
+ public static CursorBuildSpec makeCursorBuildSpec(TopNQuery query, @Nullable QueryMetrics<?> queryMetrics)
+ {
+ // virtual column is currently only used as a decorator to pass to the cursor holder to allow specializing cursor
+ // and vector cursors if any pre-aggregated data at the matching granularity is available
+ // eventually this could probably be reworked to be used by the granularizer instead of the existing method
+ // of creating a selector on the time column
+ final VirtualColumn granularityVirtual = Granularities.toVirtualColumn(query);
Review Comment:
I think i've got a better solution for this, moved the logic into `Granularities.decorateCursorBuildSpec` which takes the `CursorBuildSpec` with the expected query stuff and adds an additional grouping and virtual column if granularity is not ALL, which seems nicer, and a lot less strange than building it into `CursorBuildSpec` like I had it before.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1718036738
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -154,10 +165,26 @@ public boolean shouldVectorize(final boolean canVectorize)
return true;
}
+
+ @Override
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
+ {
+ if (!canVectorize) {
+ cleanup.run();
+ throw new ISE("Cannot vectorize!");
+ }
+
+ return true;
+ }
};
public abstract boolean shouldVectorize(boolean canVectorize);
+ public boolean shouldVectorize(boolean canVectorize, Runnable cleanup)
Review Comment:
removed this stuff
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702431223
##########
processing/src/main/java/org/apache/druid/segment/CursorMaker.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.UOE;
+import org.apache.druid.segment.vector.VectorCursor;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+
+public interface CursorMaker extends Closeable
Review Comment:
changed the names, I haven't yet switched the `CursorHolder` implementations to stash cursor/vector cursor yet, though looking into it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702431318
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
+ */
boolean isDone();
+
+ /**
+ * Check if there are any additional rows in the cursor, or if the thread has been interrupted
+ */
boolean isDoneOrInterrupted();
+
+ /**
+ * Mark a position on the cursor which can recalled with {@link #resetToMark()}
+ */
+ void mark(DateTime mark);
Review Comment:
updated javadocs to try to clarify the contract and usage
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1695771857
##########
processing/src/main/java/org/apache/druid/segment/StorageAdapter.java:
##########
@@ -35,6 +37,59 @@
@PublicApi
public interface StorageAdapter extends CursorFactory, ColumnInspector
{
+
+ /**
+ * Build a {@link CursorMaker} which can provide {@link Cursor} and {@link VectorCursor} (if capable) which allows
+ * scanning segments and creating {@link ColumnSelectorFactory} and
+ * {@link org.apache.druid.segment.vector.VectorColumnSelectorFactory} respectively to read row values.
+ */
+ @Override
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return StorageAdapter.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Cursor makeCursor()
+ {
+ return Iterables.getOnlyElement(
+ StorageAdapter.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ ).toList()
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeCursors](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7633)
##########
processing/src/main/java/org/apache/druid/segment/StorageAdapter.java:
##########
@@ -35,6 +37,59 @@
@PublicApi
public interface StorageAdapter extends CursorFactory, ColumnInspector
{
+
+ /**
+ * Build a {@link CursorMaker} which can provide {@link Cursor} and {@link VectorCursor} (if capable) which allows
+ * scanning segments and creating {@link ColumnSelectorFactory} and
+ * {@link org.apache.druid.segment.vector.VectorColumnSelectorFactory} respectively to read row values.
+ */
+ @Override
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return StorageAdapter.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.canVectorize](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7632)
##########
processing/src/main/java/org/apache/druid/segment/StorageAdapter.java:
##########
@@ -35,6 +37,59 @@
@PublicApi
public interface StorageAdapter extends CursorFactory, ColumnInspector
{
+
+ /**
+ * Build a {@link CursorMaker} which can provide {@link Cursor} and {@link VectorCursor} (if capable) which allows
+ * scanning segments and creating {@link ColumnSelectorFactory} and
+ * {@link org.apache.druid.segment.vector.VectorColumnSelectorFactory} respectively to read row values.
+ */
+ @Override
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return StorageAdapter.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Cursor makeCursor()
+ {
+ return Iterables.getOnlyElement(
+ StorageAdapter.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ ).toList()
+ );
+ }
+
+ @Override
+ public VectorCursor makeVectorCursor()
+ {
+ return StorageAdapter.this.makeVectorCursor(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ spec.isDescending(),
+ spec.getQueryContext().getVectorSize(),
+ spec.getQueryMetrics()
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeVectorCursor](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7634)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702430986
##########
processing/src/main/java/org/apache/druid/query/CursorGranularizer.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.DateTime;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+
+public class CursorGranularizer
Review Comment:
updated the javadocs for both
##########
processing/src/main/java/org/apache/druid/query/CursorGranularizer.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.DateTime;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+
+public class CursorGranularizer
+{
+ @Nullable
+ public static CursorGranularizer create(
+ final StorageAdapter storageAdapter,
+ final Cursor cursor,
+ final Granularity granularity,
+ final Interval queryInterval,
+ final boolean descending
+ )
+ {
+ final DateTime minTime = storageAdapter.getMinTime();
+ final DateTime maxTime = storageAdapter.getMaxTime();
+
+ final Interval storageAdapterInterval = new Interval(minTime, granularity.bucketEnd(maxTime));
+ final Interval clippedQueryInterval = queryInterval.overlap(storageAdapterInterval);
+
+ if (clippedQueryInterval == null) {
+ return null;
+ }
+
+ Iterable<Interval> bucketIterable = granularity.getIterable(clippedQueryInterval);
+ if (descending) {
+ bucketIterable = Lists.reverse(ImmutableList.copyOf(bucketIterable));
+ }
+ final Interval firstBucket = granularity.bucket(clippedQueryInterval.getStart());
+
+ final ColumnValueSelector timeSelector;
+ if (firstBucket.contains(clippedQueryInterval)) {
+ // Only one bucket, no need to read the time column.
+ assert Iterables.size(bucketIterable) == 1;
+ timeSelector = null;
+ } else {
+ // Multiple buckets, need to read the time column to know when we move from one to the next.
+ timeSelector = cursor.getColumnSelectorFactory().makeColumnValueSelector(ColumnHolder.TIME_COLUMN_NAME);
+ }
+
+ return new CursorGranularizer(cursor, bucketIterable, timeSelector, descending);
+ }
+
+ // And a cursor that has been made from it.
Review Comment:
removed
##########
processing/src/main/java/org/apache/druid/query/FrameBasedInlineDataSource.java:
##########
@@ -81,18 +82,22 @@ public RowSignature getRowSignature()
public Sequence<Object[]> getRowsAsSequence()
{
-
+ final Closer closer = Closer.create();
final Sequence<Cursor> cursorSequence =
Sequences.simple(frames)
- .flatMap(
+ .map(
Review Comment:
changed, though might be nice to make some special `Sequence` for a singleton so we don't have to make a singleton list and do the iterator maker and all that junk, but I don't feel like doing that in this PR.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704640053
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
+ */
boolean isDone();
+
+ /**
+ * Check if there are any additional rows in the cursor, or if the thread has been interrupted
+ */
boolean isDoneOrInterrupted();
+
+ /**
+ * Mark a position on the cursor which can recalled with {@link #resetToMark()}
+ */
+ void mark(DateTime mark);
Review Comment:
made another attempt at this, i think maybe its clear 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702047874
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/util/ToObjectVectorColumnProcessorFactoryTest.java:
##########
@@ -54,7 +57,8 @@ public void setUp()
@Test
public void testRead()
{
- try (final VectorCursor cursor = makeCursor()) {
+ try (final CursorMaker maker = makeCursor()) {
Review Comment:
`makeCursorMaker()`?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -294,15 +289,22 @@ protected ReturnOrAwait<Unit> runWithInputChannel(
final Frame frame = inputChannel.read();
final FrameSegment frameSegment = new FrameSegment(frame, inputFrameReader, SegmentId.dummy("scan"));
- final long rowsFlushed = setNextCursor(
- Iterables.getOnlyElement(
- makeCursors(
- query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(Intervals.ONLY_ETERNITY)),
- mapSegment(frameSegment).asStorageAdapter()
- ).toList()
- ),
- frameSegment
- );
+ final StorageAdapter adapter = mapSegment(frameSegment).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
+
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor nextCursor = maker.makeCursor();
+
+ if (nextCursor == null) {
+ // no cursor
+ maker.close();
Review Comment:
`maker` is already registered with the `closer`, so I think there's no need to do `maker.close()` here. The old code hadn't registered the `cursorYielder` at this point.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
Since we're changing the interfaces, better to drop `descending` and replace it with `sortOrder`. That's useful for projections since different projections might have different sort orders.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -252,21 +245,23 @@ protected ReturnOrAwait<Unit> runWithSegment(final SegmentWithDescriptor segment
if (cursor == null) {
final ResourceHolder<Segment> segmentHolder = closer.register(segment.getOrLoad());
- final Yielder<Cursor> cursorYielder = Yielders.each(
- makeCursors(
- query.withQuerySegmentSpec(new SpecificSegmentSpec(segment.getDescriptor())),
- mapSegment(segmentHolder.get()).asStorageAdapter()
- )
- );
+ final StorageAdapter adapter = mapSegment(segmentHolder.get()).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
- if (cursorYielder.isDone()) {
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor nextCursor = maker.makeCursor();
+
+ if (nextCursor == null) {
// No cursors!
- cursorYielder.close();
+ maker.close();
Review Comment:
`maker` is already registered with the `closer`, so I think there's no need to do `maker.close()` here. The old code hadn't registered the `cursorYielder` at this point.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
+ {
+ return descending;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static class CursorBuildSpecBuilder
+ {
+ @Nullable
+ private Filter filter;
+ private Interval interval = Intervals.ETERNITY;
+ private Granularity granularity = Granularities.NONE;
+
+ @Nullable
+ private List<String> groupingColumns = null;
+ private VirtualColumns virtualColumns = VirtualColumns.EMPTY;
+ @Nullable
+ private List<AggregatorFactory> aggregators = null;
+ private boolean descending = false;
+
+ private QueryContext queryContext = QueryContext.empty();
+ @Nullable
+ private QueryMetrics<?> queryMetrics;
+
+ private CursorBuildSpecBuilder()
+ {
+ //
Review Comment:
`//` what? Possibly should be `// Do nothing.`
##########
processing/src/main/java/org/apache/druid/query/FrameBasedInlineDataSource.java:
##########
@@ -81,18 +82,22 @@ public RowSignature getRowSignature()
public Sequence<Object[]> getRowsAsSequence()
{
-
+ final Closer closer = Closer.create();
final Sequence<Cursor> cursorSequence =
Sequences.simple(frames)
- .flatMap(
+ .map(
Review Comment:
IMO, it's better closeable management to use a `flatMap` with single-cursor sequences that individually do `withBaggage(maker)`. That way the makers get closed as soon as we're done with them, instead of all at the end.
It doesn't really matter right now for frames, but I'm thinking in case it does matter in the future.
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorMaker.java:
##########
@@ -668,4 +724,43 @@
return new DescendingTimestampCheckingOffset(baseOffset.clone(), timestamps, timeLimit, allWithinThreshold);
}
}
+
+ private final class CursorResources implements Closeable
Review Comment:
seems like a good point ☝️
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
+ */
boolean isDone();
+
+ /**
+ * Check if there are any additional rows in the cursor, or if the thread has been interrupted
+ */
boolean isDoneOrInterrupted();
+
+ /**
+ * Mark a position on the cursor which can recalled with {@link #resetToMark()}
+ */
+ void mark(DateTime mark);
+
+ /**
+ * Reset to position set by {@link #mark(DateTime)}
+ */
+ void resetToMark();
+
+ /**
+ * Reset to start of cursor
Review Comment:
Should specify that this also clears the mark.
##########
processing/src/main/java/org/apache/druid/segment/CursorMaker.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.UOE;
+import org.apache.druid.segment.vector.VectorCursor;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+
+public interface CursorMaker extends Closeable
Review Comment:
I wonder if `CursorHolder` is a better name than `CursorMaker`? Using "holder" rather than "maker" better emphasizes the fact that the `CursorMaker` (or `CursorHolder`) is closeable— it suggests it's a container that wraps a `Cursor` or `VectorCursor` + the resources used by the cursor. It also avoids the awkward name `CursorMakerFactory`.
If we did that, I think we'd also change:
- `makeCursor` -> `asCursor()` or `getCursor()`
- `makeVectorCursor` -> `asVectorCursor()` or `getVectorCursor()`
- `CursorMakerFactory` -> `CursorHolderFactory` or even stick with `CursorFactory`
- `CursorMakerFactory#asCursorMaker` -> `#makeCursorHolder`
- possibly update `CursorMaker` (`CursorHolder`) implementations to stash the cursor in a local field, so `asCursor()` or `getCursor()` could be called multiple times and return the same cursor. This would enable query engines to pass around the `CursorHolder` as a unified `Closeable` + holder for the actual cursor, which would potentially simplify the logic around tracking that things are closed properly.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
Review Comment:
Something is odd with the API design here. With the changes to cursor sequences, I would think that granularity should no longer be a concept that the cursors need to think about. It should be something that is dealt with at the query engine level. In fact, `getGranularity()` doesn't appear to be used by the `CursorMaker` implementations, except sometimes to throw exceptions saying that granularities other than `ALL` aren't supported.
It may be that this is here to support future work with projections. If that's the reason for having it, I would recommend we _don't_ use `granularity` in determining what projections to use; better to use expressions or virtual columns. Like, rather than encoding that a projection has `granularity: hour`, we should encode that it has a dimension `timestamp_floor(__time, 'PT1H')`. The `granularity` concept involves too much weirdness and it's best to have it totally outside the cursor logic.
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
+ */
boolean isDone();
+
+ /**
+ * Check if there are any additional rows in the cursor, or if the thread has been interrupted
+ */
boolean isDoneOrInterrupted();
+
+ /**
+ * Mark a position on the cursor which can recalled with {@link #resetToMark()}
+ */
+ void mark(DateTime mark);
Review Comment:
This should include more info on the `mark` parameter. It appears to be used by `IncrementalIndexCursor` and `RowBasedCursor`. But other popular implementations (`FrameCursor`, `QueryableIndexCursor`) ignore it. Query engines need to be written in such a way that they work properly on all cursor types, so we should have some commentary on how to properly use this method, and what behavioral guarantees apply.
##########
processing/src/main/java/org/apache/druid/segment/CursorMaker.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.UOE;
+import org.apache.druid.segment.vector.VectorCursor;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+
+public interface CursorMaker extends Closeable
+{
+ /**
+ * Create a {@link Cursor} for use with non-vectorized query engines.
+ */
+ @Nullable
+ Cursor makeCursor();
+
+ /**
+ * Create a {@link VectorCursor} for use with vectorized query engines.
+ */
+ @Nullable
+ default VectorCursor makeVectorCursor()
+ {
+ throw new UOE("Cannot vectorize. Check 'canVectorize' before calling 'makeVectorCursor' on %s.", this.getClass().getName());
+ }
+
+ /**
+ * Returns true if this {@link CursorMaker} supports calling {@link #makeVectorCursor()}.
+ */
+ default boolean canVectorize()
+ {
+ return false;
+ }
+
+ /**
+ * Release any resources acquired by cursors.
+ */
+ @Override
+ default void close()
+ {
+ // nothing to close
+ }
+
+ CursorMaker EMPTY = new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return true;
+ }
+
+ @Override
+ public Cursor makeCursor()
Review Comment:
`@Nullable`
##########
processing/src/main/java/org/apache/druid/query/CursorGranularizer.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.DateTime;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+
+public class CursorGranularizer
+{
+ @Nullable
+ public static CursorGranularizer create(
+ final StorageAdapter storageAdapter,
+ final Cursor cursor,
+ final Granularity granularity,
+ final Interval queryInterval,
+ final boolean descending
+ )
+ {
+ final DateTime minTime = storageAdapter.getMinTime();
+ final DateTime maxTime = storageAdapter.getMaxTime();
+
+ final Interval storageAdapterInterval = new Interval(minTime, granularity.bucketEnd(maxTime));
+ final Interval clippedQueryInterval = queryInterval.overlap(storageAdapterInterval);
+
+ if (clippedQueryInterval == null) {
+ return null;
+ }
+
+ Iterable<Interval> bucketIterable = granularity.getIterable(clippedQueryInterval);
+ if (descending) {
+ bucketIterable = Lists.reverse(ImmutableList.copyOf(bucketIterable));
+ }
+ final Interval firstBucket = granularity.bucket(clippedQueryInterval.getStart());
+
+ final ColumnValueSelector timeSelector;
+ if (firstBucket.contains(clippedQueryInterval)) {
+ // Only one bucket, no need to read the time column.
+ assert Iterables.size(bucketIterable) == 1;
+ timeSelector = null;
+ } else {
+ // Multiple buckets, need to read the time column to know when we move from one to the next.
+ timeSelector = cursor.getColumnSelectorFactory().makeColumnValueSelector(ColumnHolder.TIME_COLUMN_NAME);
+ }
+
+ return new CursorGranularizer(cursor, bucketIterable, timeSelector, descending);
+ }
+
+ // And a cursor that has been made from it.
Review Comment:
This comment doesn't make sense.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
+ {
+ return descending;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static class CursorBuildSpecBuilder
+ {
+ @Nullable
+ private Filter filter;
+ private Interval interval = Intervals.ETERNITY;
+ private Granularity granularity = Granularities.NONE;
+
+ @Nullable
+ private List<String> groupingColumns = null;
Review Comment:
nit: no reason to have `= null`, fields are initialized to `null` anyway. (like `filter`)
##########
processing/src/main/java/org/apache/druid/segment/Cursor.java:
##########
@@ -35,11 +36,45 @@
*/
public interface Cursor
{
+ /**
+ * Get a {@link ColumnSelectorFactory} whose selectors will be backed by the row values at the current position of
+ * the cursor
+ */
ColumnSelectorFactory getColumnSelectorFactory();
- DateTime getTime();
+
+ /**
+ * Advance to the next row in the cursor, checking if thread has been interrupted after advancing and possibly
+ * throwing {@link QueryInterruptedException} if so
+ */
void advance();
+
+ /**
+ * Advance to the next row in the cursor
+ */
void advanceUninterruptibly();
+
+ /**
+ * Check if there are any additional rows in the cursor
Review Comment:
IMO the meaning of `isDone` is a little different than this text suggests. The text makes it sound like `Iterator#hasNext`, but the behavior is subtly different. The `isDone` method is not checking if there are additional rows after the current cursor position; it checks if the current cursor position is valid. That is, expected usage is:
```
while (!cursor.isDone()) {
// do stuff
cursor.advance();
}
```
Rather than the typical iterator pattern:
```
while (iter.hasNext()) {
T obj = iter.next();
// do stuff
}
```
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorMaker.java:
##########
@@ -483,19 +471,13 @@ public ColumnSelectorFactory getColumnSelectorFactory()
return columnSelectorFactory;
}
- @Override
- public DateTime getTime()
- {
- return bucketStart;
- }
-
@Override
public void advance()
{
cursorOffset.increment();
// Must call BaseQuery.checkInterrupted() after cursorOffset.increment(), not before, because
// FilteredOffset.increment() is a potentially long, not an "instant" operation (unlike to all other subclasses
- // of Offset) and it returns early on interruption, leaving itself in an illegal state. We should not let
+ // of Offset) and it returns early on interruption, leaving itself in an illegal We should not let
Review Comment:
the `state.` seems to have been removed erroneously
##########
processing/src/main/java/org/apache/druid/query/vector/VectorCursorGranularizer.java:
##########
@@ -102,6 +71,37 @@ public static VectorCursorGranularizer create(
return new VectorCursorGranularizer(cursor, bucketIterable, timeSelector);
}
+ // And a cursor that has been made from it.
Review Comment:
This comment didn't make sense before, and it still doesn't make sense ;P
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -271,4 +275,24 @@ default Set<String> getRequiredColumns()
{
return null;
}
+
+ default CursorBuildSpec asCursorBuildSpec(@Nullable QueryMetrics<?> queryMetrics)
+ {
+ final List<Interval> intervals = getIntervals();
+ if (intervals.size() > 1) {
Review Comment:
should be `!= 1`, due to the `Iterables.getOnlyElement` later.
BTW, the error message and only-element retrieval can be combined, if you like, using `CollectionUtils.getOnlyElement` (which also takes an error supplier).
##########
processing/src/main/java/org/apache/druid/query/CursorGranularizer.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.segment.ColumnValueSelector;
+import org.apache.druid.segment.Cursor;
+import org.apache.druid.segment.StorageAdapter;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.DateTime;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+
+public class CursorGranularizer
Review Comment:
This class should have some javadocs explaining its purpose and should have a `@see` for `VectorCursorGranularizer`. Also the javadocs for `VectorCursorGranularizer` need to be updated; they still say that "Nonvectorized engines have it handled for them by the StorageAdapter".
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702313202
##########
processing/src/main/java/org/apache/druid/segment/CursorMaker.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.UOE;
+import org.apache.druid.segment.vector.VectorCursor;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+
+public interface CursorMaker extends Closeable
Review Comment:
sounds good to me, `CursorMaker` was the name before I reworked things to allow it to be closeable instead of the strange mixed model of the old thing where `Sequence<Cursor>` closed itself by consuming the sequence and `VectorCursor` was closeable.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710181883
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -32,35 +33,59 @@
* Interface extended by {@link StorageAdapter}, which gives them the power to create cursors.
*
* @see StorageAdapter
+ *
+ * @deprecated This interface is deprecated and no longer implemented by any built-in {@link StorageAdapter}. Callers
+ * should use {@link CursorMakerFactory#makeCursorHolder(CursorBuildSpec)} instead. Implementors should implement
+ * {@link CursorMakerFactory#makeCursorHolder(CursorBuildSpec)} instead.
*/
+@Deprecated
Review Comment:
I have a feeling we will be evolving the `CursorHolderFactory` interfaces over the next couple of releases as we build out projections, and possibly adjust how granularity and sort ordering is handled. So it's IMO early to deprecate this interface. Extension `Query` and `Segment` can/should keep using `CursorFactory` until the new interfaces stabilize.
##########
processing/src/main/java/org/apache/druid/segment/FilteredStorageAdapter.java:
##########
@@ -45,26 +42,21 @@ public FilteredStorageAdapter(final StorageAdapter adapter, final DimFilter filt
}
@Override
- public Sequence<Cursor> makeCursors(
- @Nullable Filter filter,
- Interval interval,
- VirtualColumns virtualColumns,
- Granularity gran,
- boolean descending,
- @Nullable QueryMetrics<?> queryMetrics
- )
+ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
{
- final Filter andFilter;
- if (filter == null) {
+ final CursorBuildSpec.CursorBuildSpecBuilder buildSpecBuilder = CursorBuildSpec.builder(spec);
+ final Filter newFilter;
+ if (spec.getFilter() == null) {
if (filterOnDataSource != null) {
Review Comment:
Odd that this handles the case where `filterOnDataSource == null`, but the other branch doesn't. Perhaps it can't be null, and we can simplify this code by removing the null check.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.scan.Order;
+import org.apache.druid.query.scan.OrderBy;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+ @Nullable
+ private final List<OrderBy> orderByColumns;
+
+ private final QueryContext queryContext;
+
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ @Nullable List<OrderBy> preferredOrdering,
+ QueryContext queryContext,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.orderByColumns = preferredOrdering;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ @Nullable
+ public List<OrderBy> getPreferredOrdering()
+ {
+ return orderByColumns;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static boolean preferDescendingTimeOrder(@Nullable List<OrderBy> preferredOrdering)
+ {
+ if (preferredOrdering != null && preferredOrdering.size() == 1) {
Review Comment:
This returns `false` for a preferred ordering that starts with `__time` but also has some other stuff after it. Is that good?
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.scan.Order;
+import org.apache.druid.query.scan.OrderBy;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+ @Nullable
+ private final List<OrderBy> orderByColumns;
+
+ private final QueryContext queryContext;
+
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ @Nullable List<OrderBy> preferredOrdering,
+ QueryContext queryContext,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.orderByColumns = preferredOrdering;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ @Nullable
+ public List<OrderBy> getPreferredOrdering()
+ {
+ return orderByColumns;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static boolean preferDescendingTimeOrder(@Nullable List<OrderBy> preferredOrdering)
+ {
+ if (preferredOrdering != null && preferredOrdering.size() == 1) {
+ final OrderBy orderBy = Iterables.getOnlyElement(preferredOrdering);
+ return ColumnHolder.TIME_COLUMN_NAME.equals(orderBy.getColumnName()) && Order.DESCENDING == orderBy.getOrder();
+ }
+ return false;
+ }
+
+ public static class CursorBuildSpecBuilder
+ {
+ @Nullable
+ private Filter filter;
+ private Interval interval = Intervals.ETERNITY;
+
+ @Nullable
+ private List<String> groupingColumns;
+ private VirtualColumns virtualColumns = VirtualColumns.EMPTY;
+ @Nullable
+ private List<AggregatorFactory> aggregators;
+ @Nullable
+ private List<OrderBy> preferredOrdering;
+
+ private QueryContext queryContext = QueryContext.empty();
+ @Nullable
+ private QueryMetrics<?> queryMetrics;
+
+ private CursorBuildSpecBuilder()
+ {
+ // initialize with defaults
+ }
+
+ private CursorBuildSpecBuilder(CursorBuildSpec buildSpec)
+ {
+ this.filter = buildSpec.filter;
+ this.interval = buildSpec.interval;
+ this.groupingColumns = buildSpec.groupingColumns;
+ this.virtualColumns = buildSpec.virtualColumns;
+ this.aggregators = buildSpec.aggregators;
+ this.preferredOrdering = buildSpec.orderByColumns;
+ this.queryContext = buildSpec.queryContext;
+ this.queryMetrics = buildSpec.queryMetrics;
+ }
+
+ public CursorBuildSpecBuilder setFilter(@Nullable Filter filter)
+ {
+ this.filter = filter;
+ return this;
+ }
+
+ public CursorBuildSpecBuilder setInterval(Interval interval)
+ {
+ this.interval = interval;
+ return this;
+ }
+
+ public CursorBuildSpecBuilder setGroupingAndVirtualColumns(
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns
+ )
+ {
+ final VirtualColumn granularityVirtual = Granularities.toVirtualColumn(granularity);
Review Comment:
It seems a little odd that the builder is deciding what the name and specific contents of the virtual column will be.
I think we can leave this for now, I'm happy enough that the `CursorBuildSpec` itself doesn't have a `granularity` field. But IMO it'd make more sense to push the `granularity` all the way to the query engines, so they build the virtual column rather than the `CursorBuildSpecBuilder` doing it. In tandem with that we should also do this stuff:
- Adjust the `timestamp_floor` implementation to take advantage of runs in the `__time` column. Rather than doing `granularity.bucketStart` on each row, it should keep some state about the current bucket start/end and compare the `__time` to that current start/end.
- Adjust the Granularizer logic to do equality checks on the virtual granularity column, rather than greater/less-than comparisons. With the virtual column always reporting the exact start of the bucket, equality is good enough.
That should enable query engines to use the granularity virtual column approach 100% of the time without suffering perf loss.
We can do all this later though. If you agree, feel free to keep things the way they are in this patch, just leave a comment here to note the future change. Also write in the javadoc for this particular method that it's unstable & currently unused, so callers in extensions should stick to `setVirtualColumns`. (Which actually does do something today.)
##########
processing/src/test/java/org/apache/druid/frame/segment/FrameStorageAdapterTest.java:
##########
@@ -345,58 +367,46 @@ public void tearDown()
}
@Test
- public void test_makeCursors()
+ public void test_makeCursor()
{
- assertCursorsMatch(
- adapter ->
- adapter.makeCursors(
- filter,
- interval,
- virtualColumns,
- Granularities.ALL, // Frames only support Granularities.ALL: no point testing the others.
- descending,
- null
- )
- );
+ assertCursorMatch(adapter -> adapter.makeCursorHolder(buildSpec));
}
@Test
public void test_makeVectorCursor()
{
- Assume.assumeTrue(frameAdapter.canVectorize(filter, virtualColumns, descending));
-
- assertVectorCursorsMatch(
- adapter ->
- adapter.makeVectorCursor(
- filter,
- interval,
- virtualColumns,
- descending,
- VECTOR_SIZE,
- null
- )
- );
+ assertVectorCursorsMatch(adapter -> adapter.makeCursorHolder(buildSpec));
}
- private void assertCursorsMatch(final Function<StorageAdapter, Sequence<Cursor>> call)
+ private void assertCursorMatch(final Function<StorageAdapter, CursorHolder> call)
{
final RowSignature signature = frameAdapter.getRowSignature();
- final Sequence<List<Object>> queryableRows =
- call.apply(queryableAdapter).flatMap(cursor -> FrameTestUtil.readRowsFromCursor(cursor, signature));
- final Sequence<List<Object>> frameRows =
- call.apply(frameAdapter)
- .flatMap(cursor -> FrameTestUtil.readRowsFromCursor(advanceAndReset(cursor), signature));
- FrameTestUtil.assertRowsEqual(queryableRows, frameRows);
+ try (final CursorHolder queryableMaker = call.apply(queryableAdapter);
+ final CursorHolder frameMaker = call.apply(frameAdapter)) {
+ final Sequence<List<Object>> queryableRows =
+ FrameTestUtil.readRowsFromCursor(queryableMaker.asCursor(), signature);
+ final Sequence<List<Object>> frameRows =
+ FrameTestUtil.readRowsFromCursor(frameMaker.asCursor(), signature);
Review Comment:
After this change, `advanceAndReset(Cursor)` is no longer used. Is that intentional? If not, please add it back in; it's meant to be a no-op that helps test that `reset()` works properly. (Unless you've added some other tests that exercise `reset()` better.)
##########
processing/src/main/java/org/apache/druid/segment/CursorMakerFactory.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * 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;
+
+public interface CursorMakerFactory
Review Comment:
`CursorHolderFactory`
##########
processing/src/main/java/org/apache/druid/segment/RowBasedCursor.java:
##########
@@ -140,4 +148,12 @@ private void advanceToMatchingRow()
rowId++;
}
}
+
+ private void advanceToMatchingMarkRow()
+ {
+ while (!isDone() && rowId < markId) {
Review Comment:
This should be checking `valueMatcher` somehow, right? I think as written, it will advance `markId` number of rows on the underlying `rowWalker`, without regard for which ones match the provided filter. (Normally, `rowId` is incremented once for each row that matches the filter.)
Additionally, this logic creates a requirement that `rowWalker` be immutable. I don't think that'll always be true. For example, we use this cursor on top of external data, where re-walking the walker means re-fetching a file. The file could have changed. We also use it on top of lookups, where re-walking could get a different set of keys. I am not sure how really to do sensible marking on data streams like this.
##########
processing/src/main/java/org/apache/druid/query/scan/OrderBy.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.query.scan;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.StringUtils;
+
+import java.util.Objects;
+
+public class OrderBy
Review Comment:
Move outside `scan`? Possibly to the `org.apache.druid.query` package.
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.scan.Order;
+import org.apache.druid.query.scan.OrderBy;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+ @Nullable
+ private final List<OrderBy> orderByColumns;
+
+ private final QueryContext queryContext;
+
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ @Nullable List<OrderBy> preferredOrdering,
+ QueryContext queryContext,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.orderByColumns = preferredOrdering;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ @Nullable
+ public List<OrderBy> getPreferredOrdering()
+ {
+ return orderByColumns;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static boolean preferDescendingTimeOrder(@Nullable List<OrderBy> preferredOrdering)
Review Comment:
This method could use some javadoc. Everything in this class could, really. But this one especially, since it's not obvious from the name what it does.
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinEngine.java:
##########
@@ -250,12 +244,32 @@ public boolean isDoneOrInterrupted()
return isDone() || Thread.currentThread().isInterrupted();
}
+ @Override
+ public void mark()
+ {
+ joinMarkId = joinColumnSelectorFactory.getRowId();
+ }
+
+ @Override
+ public void resetToMark()
+ {
+ leftCursor.reset();
+ joinMatcher.reset();
+ joinColumnSelectorFactory.resetRowId();
+ initialize();
+ while (!isDone() && joinColumnSelectorFactory.getRowId() < joinMarkId) {
Review Comment:
I wonder if this approach, where we fully reset and advance `leftCursor`, will work properly on top of mutable `leftCursor` such as `IncrementalIndexCursor`. Seems like it would be safer to pass through `mark` and `resetToMark` to the `leftCursor`.
Speaking of mutability, the rhs Joinable could also be mutable, like if it's a lookup backed by a Kafka topic. We don't snapshot those. That's also problematic for this logic, since it could make the `rowId` inconsistent post-reset.
I don't have a good solution in mind for problems like this. The simplest one I can think of right now is to side-step the problem by making it so not all cursors support `mark()` (for example, a join cursor might not support it) & then adjust TopN to use `runWithCardinalityUnknown` if the underlying cursor doesn't support `mark()` and `params.getNumValuesPerPass()` is less than `cardinality`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702318059
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
should i re-use an existing sort order thingy or make a new one? it seems like we've got a few...
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704511990
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
cool, calling it `preferredOrdering` is what pulls it all together I think, pushed up some changes
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #16533:
URL: https://github.com/apache/druid/pull/16533
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2290199127
The main cursor interfaces look good to me as of the latest patch, so my most recent review is more about the call sites and query engine adjustments.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259449370
TimeseriesBenchmark.querySingleIncrementalIndex
master:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.A avgt 5 2182720.903 ± 28600.656 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterNumeric avgt 5 170181.705 ± 4949.065 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterAlphanumeric avgt 5 235274.971 ± 2933.406 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterByInterval avgt 5 0.942 ± 0.010 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.A avgt 5 2186282.510 ± 53491.354 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterNumeric avgt 5 171208.544 ± 3533.276 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterAlphanumeric avgt 5 268220.782 ± 3266.675 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterByInterval avgt 5 0.709 ± 0.017 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.A avgt 5 2032495.610 ± 51809.387 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterNumeric avgt 5 48984.489 ± 581.160 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterAlphanumeric avgt 5 97106.597 ± 1876.155 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterByInterval avgt 5 0.966 ± 0.015 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.A avgt 5 2078413.440 ± 600339.575 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterNumeric avgt 5 46754.332 ± 850.925 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterAlphanumeric avgt 5 96115.215 ± 1553.669 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterByInterval avgt 5 0.668 ± 0.007 us/op
```
PR:
```
Benchmark (descending) (indexType) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) Mode Cnt Score Error Units
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.A avgt 5 2100705.332 ± 11924.334 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterNumeric avgt 5 195322.564 ± 2663.217 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterAlphanumeric avgt 5 257597.411 ± 1110.743 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A all 750000 basic.timeFilterByInterval avgt 5 0.896 ± 0.004 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.A avgt 5 2119013.258 ± 43279.983 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterNumeric avgt 5 188110.527 ± 1529.513 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterAlphanumeric avgt 5 234716.762 ± 1679.083 us/op
TimeseriesBenchmark.querySingleIncrementalIndex true onheap N/A hour 750000 basic.timeFilterByInterval avgt 5 0.653 ± 0.010 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.A avgt 5 1970874.151 ± 22219.028 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterNumeric avgt 5 50425.220 ± 370.576 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterAlphanumeric avgt 5 93658.840 ± 1354.271 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A all 750000 basic.timeFilterByInterval avgt 5 0.975 ± 0.012 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.A avgt 5 1968549.097 ± 10580.986 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterNumeric avgt 5 52729.201 ± 415.241 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterAlphanumeric avgt 5 97213.046 ± 1565.803 us/op
TimeseriesBenchmark.querySingleIncrementalIndex false onheap N/A hour 750000 basic.timeFilterByInterval avgt 5 0.662 ± 0.003 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #16533:
URL: https://github.com/apache/druid/pull/16533#issuecomment-2259193809
GroupByBenchmark
master:
```
Benchmark (indexType) (initialBuckets) (numProcessingThreads) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) (vectorize) Mode Cnt Score Error Units
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 all 100000 basic.A false avgt 5 114660.922 ± 5464.284 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 all 100000 basic.nested false avgt 5 123547.867 ± 5696.161 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 day 100000 basic.A false avgt 5 121517.446 ± 28257.788 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 day 100000 basic.nested false avgt 5 131095.237 ± 16358.979 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A all 100000 basic.A false avgt 5 39179.291 ± 1238.116 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A all 100000 basic.nested false avgt 5 44367.471 ± 1390.506 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A day 100000 basic.A false avgt 5 38718.212 ± 736.095 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A day 100000 basic.nested false avgt 5 45114.878 ± 989.669 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 all 100000 basic.A false avgt 5 20979.467 ± 457.549 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 all 100000 basic.nested false avgt 5 29598.945 ± 542.109 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 day 100000 basic.A false avgt 5 21813.519 ± 353.517 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 day 100000 basic.nested false avgt 5 29391.385 ± 588.024 us/op
```
PR:
```
Benchmark (indexType) (initialBuckets) (numProcessingThreads) (numSegments) (queryGranularity) (rowsPerSegment) (schemaAndQuery) (vectorize) Mode Cnt Score Error Units
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 all 100000 basic.A false avgt 5 107321.215 ± 6436.861 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 all 100000 basic.nested false avgt 5 128178.617 ± 2089.745 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 day 100000 basic.A false avgt 5 129252.395 ± 14078.504 us/op
GroupByBenchmark.queryMultiQueryableIndexX N/A -1 4 4 day 100000 basic.nested false avgt 5 117537.002 ± 1043.043 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A all 100000 basic.A false avgt 5 38260.686 ± 330.199 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A all 100000 basic.nested false avgt 5 44170.960 ± 930.744 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A day 100000 basic.A false avgt 5 38390.024 ± 406.843 us/op
GroupByBenchmark.querySingleIncrementalIndex onheap -1 4 N/A day 100000 basic.nested false avgt 5 44218.134 ± 616.097 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 all 100000 basic.A false avgt 5 20896.971 ± 208.873 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 all 100000 basic.nested false avgt 5 28763.159 ± 576.115 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 day 100000 basic.A false avgt 5 21956.331 ± 296.555 us/op
GroupByBenchmark.querySingleQueryableIndex N/A -1 4 4 day 100000 basic.nested false avgt 5 29390.898 ± 1362.409 us/op
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1693541054
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/LazilyDecoratedRowsAndColumns.java:
##########
@@ -280,26 +279,24 @@
sortColumns
);
- final FrameWriter frameWriter = frameWriterFactory.newFrameWriter(columnSelectorFactory);
- for (; !in.isDoneOrInterrupted() && remainingRowsToSkip > 0; remainingRowsToSkip--) {
- in.advance();
+ final FrameWriter writer = frameWriterFactory.newFrameWriter(columnSelectorFactory);
+ for (; !cursor.isDoneOrInterrupted() && remainingRowsToSkip > 0; remainingRowsToSkip--) {
+ cursor.advance();
}
- for (; !in.isDoneOrInterrupted() && remainingRowsToFetch > 0; remainingRowsToFetch--) {
- frameWriter.addSelection();
- in.advance();
+ for (; !cursor.isDoneOrInterrupted() && remainingRowsToFetch > 0; remainingRowsToFetch--) {
+ writer.addSelection();
Review Comment:
## Dereferenced variable may be null
Variable [writer](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7605)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/StorageAdapterRowsAndColumns.java:
##########
@@ -98,45 +95,35 @@
@Nonnull
private static RowsAndColumns materialize(StorageAdapter as)
{
- final Sequence<Cursor> cursors = as.makeCursors(
- null,
- Intervals.ETERNITY,
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
-
- RowSignature rowSignature = as.getRowSignature();
-
- FrameWriter writer = cursors.accumulate(null, (accumulated, in) -> {
- if (accumulated != null) {
- // We should not get multiple cursors because we set the granularity to ALL. So, this should never
- // actually happen, but it doesn't hurt us to defensive here, so we test against it.
- throw new ISE("accumulated[%s] non-null, why did we get multiple cursors?", accumulated);
+ try (final CursorMaker maker = as.asCursorMaker(CursorBuildSpec.FULL_SCAN)) {
+ final Cursor cursor = maker.makeCursor();
+
+ if (cursor == null) {
+ return new EmptyRowsAndColumns();
}
+
+ final RowSignature rowSignature = as.getRowSignature();
- final ColumnSelectorFactory columnSelectorFactory = in.getColumnSelectorFactory();
+ final ColumnSelectorFactory columnSelectorFactory = cursor.getColumnSelectorFactory();
final FrameWriterFactory frameWriterFactory = FrameWriters.makeColumnBasedFrameWriterFactory(
new ArenaMemoryAllocatorFactory(200 << 20), // 200 MB, because, why not?
rowSignature,
Collections.emptyList()
);
- final FrameWriter frameWriter = frameWriterFactory.newFrameWriter(columnSelectorFactory);
- while (!in.isDoneOrInterrupted()) {
- frameWriter.addSelection();
- in.advance();
+ final FrameWriter writer = frameWriterFactory.newFrameWriter(columnSelectorFactory);
+ while (!cursor.isDoneOrInterrupted()) {
+ writer.addSelection();
Review Comment:
## Dereferenced variable may be null
Variable [writer](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/7606)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
Review Comment:
## Dereferenced variable may be null
Variable [selector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7609)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
+ float[] floats = selector.getFloatVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ }
+ } else {
+ nulls = selector.getNullVector();
Review Comment:
## Dereferenced variable may be null
Variable [selector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7610)
##########
processing/src/main/java/org/apache/druid/segment/CursorFactory.java:
##########
@@ -33,34 +36,106 @@
*
* @see StorageAdapter
*/
-public interface CursorFactory
+public interface CursorFactory extends CursorMakerFactory
{
+ @Override
+ default CursorMaker asCursorMaker(CursorBuildSpec spec)
+ {
+ return new CursorMaker()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return CursorFactory.this.canVectorize(spec.getFilter(), spec.getVirtualColumns(), spec.isDescending());
+ }
+
+ @Override
+ public Cursor makeCursor()
+ {
+ return Iterables.getOnlyElement(
+ CursorFactory.this.makeCursors(
+ spec.getFilter(),
+ spec.getInterval(),
+ spec.getVirtualColumns(),
+ Granularities.ALL,
+ spec.isDescending(),
+ spec.getQueryMetrics()
+ ).toList()
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.makeCursors](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7613)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
+ case LONG:
nulls = selector.getNullVector();
- float[] floats = selector.getFloatVector();
+ long[] longs = selector.getLongVector();
for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
}
- } else {
- nulls = selector.getNullVector();
- double[] doubles = selector.getDoubleVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ break;
+ case DOUBLE:
+ // special case to test floats just to get coverage on getFloatVector
+ if ("float2".equals(expression)) {
+ nulls = selector.getNullVector();
+ float[] floats = selector.getFloatVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : (double) floats[i]);
+ }
+ } else {
+ nulls = selector.getNullVector();
+ double[] doubles = selector.getDoubleVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ resultsVector.add(nulls != null && nulls[i] ? null : doubles[i]);
+ }
}
- }
- break;
- case STRING:
- Object[] objects = objectSelector.getObjectVector();
- for (int i = 0; i < objectSelector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(objects[i]);
- }
- break;
- }
+ break;
+ case STRING:
+ Object[] objects = objectSelector.getObjectVector();
Review Comment:
## Dereferenced variable may be null
Variable [objectSelector](1) may be null at this access because of [this](2) assignment.
[Show more details](https://github.com/apache/druid/security/code-scanning/7607)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -433,48 +431,42 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- null,
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinality = dimSelector.getValueCardinality();
-
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- for (int i = 0; i < 1; i++) {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v2" + i)));
- }
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- int rowNumInCursor = 0;
- // and then, cursoring continues in the other thread
- while (!cursor.isDone()) {
- IndexedInts row = dimSelector.getRow();
- row.forEach(i -> Assert.assertTrue(i < cardinality));
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(2, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7614)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -497,37 +489,28 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- new DictionaryRaceTestFilter(index, timestamp),
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinality = dimSelector.getValueCardinality();
-
- int rowNumInCursor = 0;
- while (!cursor.isDone()) {
- IndexedInts row = dimSelector.getRow();
- row.forEach(i -> Assert.assertTrue(i < cardinality));
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(5, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setFilter(new DictionaryRaceTestFilter(index, timestamp))
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7615)
##########
processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapterTest.java:
##########
@@ -548,93 +531,88 @@
final StorageAdapter sa = new IncrementalIndexStorageAdapter(index);
- Sequence<Cursor> cursors = sa.makeCursors(
- null,
- Intervals.utc(timestamp - 60_000, timestamp + 60_000),
- VirtualColumns.EMPTY,
- Granularities.ALL,
- false,
- null
- );
- final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
-
- cursors
- .map(cursor -> {
- DimensionSelector dimSelector1A = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- int cardinalityA = dimSelector1A.getValueCardinality();
-
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v1")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector1B = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v2")));
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy2"), ImmutableMap.of("billy2", "v3")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector1C = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy", "billy"));
-
- DimensionSelector dimSelector2D = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy2", "billy2"));
- //index gets more rows at this point, while other thread is iterating over the cursor
- try {
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy"), ImmutableMap.of("billy", "v3")));
- index.add(new MapBasedInputRow(timestamp, Collections.singletonList("billy3"), ImmutableMap.of("billy3", "")));
- }
- catch (Exception ex) {
- throw new RuntimeException(ex);
- }
-
- DimensionSelector dimSelector3E = cursor
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec("billy3", "billy3"));
-
- int rowNumInCursor = 0;
- // and then, cursoring continues in the other thread
- while (!cursor.isDone()) {
- IndexedInts rowA = dimSelector1A.getRow();
- rowA.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowB = dimSelector1B.getRow();
- rowB.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowC = dimSelector1C.getRow();
- rowC.forEach(i -> Assert.assertTrue(i < cardinalityA));
- IndexedInts rowD = dimSelector2D.getRow();
- // no null id, so should get empty dims array
- Assert.assertEquals(0, rowD.size());
- IndexedInts rowE = dimSelector3E.getRow();
- if (NullHandling.replaceWithDefault()) {
- Assert.assertEquals(1, rowE.size());
- // the null id
- Assert.assertEquals(0, rowE.get(0));
- } else {
- Assert.assertEquals(0, rowE.size());
- }
- cursor.advance();
- rowNumInCursor++;
- }
- Assert.assertEquals(2, rowNumInCursor);
- assertCursorsNotEmpty.incrementAndGet();
-
- return null;
- })
- .toList();
- Assert.assertEquals(1, assertCursorsNotEmpty.get());
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(Intervals.utc(timestamp - 60_000, timestamp + 60_000))
+ .setGranularity(Granularities.ALL)
+ .build();
+ try (final CursorMaker maker = sa.asCursorMaker(buildSpec)) {
+ Cursor cursor = maker.makeCursor();
+ final AtomicInteger assertCursorsNotEmpty = new AtomicInteger(0);
Review Comment:
## Unread local variable
Variable 'AtomicInteger assertCursorsNotEmpty' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7616)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -252,21 +245,23 @@
if (cursor == null) {
final ResourceHolder<Segment> segmentHolder = closer.register(segment.getOrLoad());
- final Yielder<Cursor> cursorYielder = Yielders.each(
- makeCursors(
- query.withQuerySegmentSpec(new SpecificSegmentSpec(segment.getDescriptor())),
- mapSegment(segmentHolder.get()).asStorageAdapter()
- )
- );
+ final StorageAdapter adapter = mapSegment(segmentHolder.get()).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
- if (cursorYielder.isDone()) {
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor cursor = maker.makeCursor();
Review Comment:
## Possible confusion of local and field
Confusing name: method [runWithSegment](1) also refers to field [cursor](2) (without qualifying it with 'this').
[Show more details](https://github.com/apache/druid/security/code-scanning/7617)
##########
processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVectorSelectorsTest.java:
##########
@@ -234,109 +235,100 @@
)
);
final QueryableIndexStorageAdapter storageAdapter = new QueryableIndexStorageAdapter(index);
- VectorCursor cursor = storageAdapter.makeVectorCursor(
- null,
- index.getDataInterval(),
- virtualColumns,
- false,
- 512,
- null
- );
-
- ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
-
- int rowCount = 0;
- if (capabilities.isDictionaryEncoded().isTrue()) {
- SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory().makeSingleValueDimensionSelector(
- DefaultDimensionSpec.of("v")
- );
- while (!cursor.isDone()) {
- int[] row = selector.getRowVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- results.add(selector.lookupName(row[i]));
+ final CursorBuildSpec buildSpec = CursorBuildSpec.builder()
+ .setInterval(index.getDataInterval())
+ .setGranularity(Granularities.ALL)
+ .setVirtualColumns(virtualColumns)
+ .build();
+ try (final CursorMaker maker = storageAdapter.asCursorMaker(buildSpec)) {
+ final VectorCursor cursor = maker.makeVectorCursor();
+
+ ColumnCapabilities capabilities = virtualColumns.getColumnCapabilitiesWithFallback(storageAdapter, "v");
+
+ int rowCount = 0;
+ if (capabilities.isDictionaryEncoded().isTrue()) {
+ SingleValueDimensionVectorSelector selector = cursor.getColumnSelectorFactory()
+ .makeSingleValueDimensionSelector(
+ DefaultDimensionSpec.of("v")
+ );
+ while (!cursor.isDone()) {
+ int[] row = selector.getRowVector();
+ for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
+ results.add(selector.lookupName(row[i]));
+ }
+ cursor.advance();
}
- cursor.advance();
- }
- } else {
- VectorValueSelector selector = null;
- VectorObjectSelector objectSelector = null;
- if (Types.isNumeric(outputType)) {
- selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
} else {
- objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
- }
- GroupByVectorColumnSelector groupBySelector =
- cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
- while (!cursor.isDone()) {
- final List<Object> resultsVector = new ArrayList<>();
- boolean[] nulls;
- switch (outputType.getType()) {
- case LONG:
- nulls = selector.getNullVector();
- long[] longs = selector.getLongVector();
- for (int i = 0; i < selector.getCurrentVectorSize(); i++, rowCount++) {
- resultsVector.add(nulls != null && nulls[i] ? null : longs[i]);
- }
- break;
- case DOUBLE:
- // special case to test floats just to get coverage on getFloatVector
- if ("float2".equals(expression)) {
+ VectorValueSelector selector = null;
+ VectorObjectSelector objectSelector = null;
+ if (Types.isNumeric(outputType)) {
+ selector = cursor.getColumnSelectorFactory().makeValueSelector("v");
+ } else {
+ objectSelector = cursor.getColumnSelectorFactory().makeObjectSelector("v");
+ }
+ GroupByVectorColumnSelector groupBySelector =
+ cursor.getColumnSelectorFactory().makeGroupByVectorColumnSelector("v", DeferExpressionDimensions.ALWAYS);
+ while (!cursor.isDone()) {
+ final List<Object> resultsVector = new ArrayList<>();
+ boolean[] nulls;
+ switch (outputType.getType()) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [ARRAY](1) or [COMPLEX](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/7619)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -294,15 +289,22 @@
final Frame frame = inputChannel.read();
final FrameSegment frameSegment = new FrameSegment(frame, inputFrameReader, SegmentId.dummy("scan"));
- final long rowsFlushed = setNextCursor(
- Iterables.getOnlyElement(
- makeCursors(
- query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(Intervals.ONLY_ETERNITY)),
- mapSegment(frameSegment).asStorageAdapter()
- ).toList()
- ),
- frameSegment
- );
+ final StorageAdapter adapter = mapSegment(frameSegment).asStorageAdapter();
+ if (adapter == null) {
+ throw new ISE(
+ "Null storage adapter found. Probably trying to issue a query against a segment being memory unmapped."
+ );
+ }
+
+ final CursorMaker maker = closer.register(adapter.asCursorMaker(query.asCursorBuildSpec(null)));
+ final Cursor cursor = maker.makeCursor();
Review Comment:
## Possible confusion of local and field
Confusing name: method [runWithInputChannel](1) also refers to field [cursor](2) (without qualifying it with 'this').
[Show more details](https://github.com/apache/druid/security/code-scanning/7618)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702313497
##########
processing/src/main/java/org/apache/druid/query/vector/VectorCursorGranularizer.java:
##########
@@ -102,6 +71,37 @@ public static VectorCursorGranularizer create(
return new VectorCursorGranularizer(cursor, bucketIterable, timeSelector);
}
+ // And a cursor that has been made from it.
Review Comment:
heh, will fix :+1:
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1702346817
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
Review Comment:
thinking a bit more about this i think i better see what you're saying, like I suppose I should just always represent granularity as a virtual column being grouped on in the projection, then just map native query granularity into an extra virtual column that is a grouping dimension when building the cursor build spec, will try to make that change :+1:
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1703791887
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
+ {
+ return granularity;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ public boolean isDescending()
Review Comment:
still working on this one, I have it working using scan orderBy, but i'm a bit unsure about what to do about topN and groupBy, which never set descending and do their own sorting. The problem specifically is group by, which if we order by time descending with this new logic would try to do that too the cursor, which would mean it couldn't vectorize, but the engine doesn't actually care if the results are ordered by the queries order by or not. And even if they were already ordered from a projection or something, it seems like right now no way for the engines to take advantage of this.
The workaround I have right now is basically only scan when ordered by time, timeseries, and timeboundary queries push in order by __time, everything else sets it to null. What should we do here?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1704310262
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().setGranularity(Granularities.ALL).build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ private final Granularity granularity;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+
+ private final QueryContext queryContext;
+
+ private final boolean descending;
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ Granularity granularity,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ QueryContext queryContext,
+ boolean descending,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.granularity = granularity;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.descending = descending;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ public Granularity getGranularity()
Review Comment:
> map native query granularity into an extra virtual column that is a grouping dimension when building the cursor build spec
Yeah, something like that is what I had in mind. I don't think the cursor build spec necessarily needs to 100% match what objects the query itself was created with. There can be some translation done by the query engine.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1716255570
##########
processing/src/main/java/org/apache/druid/segment/StorageAdapter.java:
##########
@@ -21,20 +21,99 @@
import com.google.common.collect.Iterables;
import org.apache.druid.guice.annotations.PublicApi;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.query.OrderBy;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.data.Indexed;
+import org.apache.druid.segment.vector.VectorCursor;
import org.joda.time.DateTime;
import org.joda.time.Interval;
import javax.annotation.Nullable;
+import java.util.List;
import java.util.Optional;
/**
*/
@PublicApi
-public interface StorageAdapter extends CursorFactory, ColumnInspector
+public interface StorageAdapter extends CursorFactory, ColumnInspector, CursorHolderFactory
{
+
+ /**
+ * Build a {@link CursorHolder} which can provide {@link Cursor} and {@link VectorCursor} (if capable) which allows
+ * scanning segments and creating {@link ColumnSelectorFactory} and
+ * {@link org.apache.druid.segment.vector.VectorColumnSelectorFactory} respectively to read row values at the cursor
+ * position.
+ */
+ @Override
+ default CursorHolder makeCursorHolder(CursorBuildSpec spec)
+ {
+ // adequate for time ordering, but needs to be updated if we support cursors ordered other time as the primary
+ final List<OrderBy> ordering;
+ final boolean descending;
+ if (Cursors.preferDescendingTimeOrdering(spec)) {
+ ordering = Cursors.descendingTimeOrder();
+ descending = true;
+ } else {
+ ordering = Cursors.ascendingTimeOrder();
+ descending = false;
+ }
+ return new CursorHolder()
+ {
+ @Override
+ public boolean canVectorize()
+ {
+ return StorageAdapter.this.canVectorize(
+ spec.getFilter(),
+ spec.getVirtualColumns(),
+ Cursors.preferDescendingTimeOrdering(spec)
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [CursorFactory.canVectorize](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/7709)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] rework cursor creation (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16533:
URL: https://github.com/apache/druid/pull/16533#discussion_r1710310601
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.QueryMetrics;
+import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.query.scan.Order;
+import org.apache.druid.query.scan.OrderBy;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.joda.time.Interval;
+
+import javax.annotation.Nullable;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class CursorBuildSpec
+{
+ public static final CursorBuildSpec FULL_SCAN = CursorBuildSpec.builder().build();
+
+ public static CursorBuildSpecBuilder builder()
+ {
+ return new CursorBuildSpecBuilder();
+ }
+
+ public static CursorBuildSpecBuilder builder(CursorBuildSpec spec)
+ {
+ return new CursorBuildSpecBuilder(spec);
+ }
+
+ @Nullable
+ private final Filter filter;
+ private final Interval interval;
+ @Nullable
+ private final List<String> groupingColumns;
+ private final VirtualColumns virtualColumns;
+ @Nullable
+ private final List<AggregatorFactory> aggregators;
+ @Nullable
+ private final List<OrderBy> orderByColumns;
+
+ private final QueryContext queryContext;
+
+ @Nullable
+ private final QueryMetrics<?> queryMetrics;
+
+ public CursorBuildSpec(
+ @Nullable Filter filter,
+ Interval interval,
+ @Nullable List<String> groupingColumns,
+ VirtualColumns virtualColumns,
+ @Nullable List<AggregatorFactory> aggregators,
+ @Nullable List<OrderBy> preferredOrdering,
+ QueryContext queryContext,
+ @Nullable QueryMetrics<?> queryMetrics
+ )
+ {
+ this.filter = filter;
+ this.interval = interval;
+ this.groupingColumns = groupingColumns;
+ this.virtualColumns = virtualColumns;
+ this.aggregators = aggregators;
+ this.orderByColumns = preferredOrdering;
+ this.queryContext = queryContext;
+ this.queryMetrics = queryMetrics;
+ }
+
+ @Nullable
+ public Filter getFilter()
+ {
+ return filter;
+ }
+
+ public Interval getInterval()
+ {
+ return interval;
+ }
+
+ @Nullable
+ public List<String> getGroupingColumns()
+ {
+ return groupingColumns;
+ }
+
+ public VirtualColumns getVirtualColumns()
+ {
+ return virtualColumns;
+ }
+
+ @Nullable
+ public List<AggregatorFactory> getAggregators()
+ {
+ return aggregators;
+ }
+
+ @Nullable
+ public List<OrderBy> getPreferredOrdering()
+ {
+ return orderByColumns;
+ }
+
+ public QueryContext getQueryContext()
+ {
+ return queryContext;
+ }
+
+ @Nullable
+ public QueryMetrics<?> getQueryMetrics()
+ {
+ return queryMetrics;
+ }
+
+ public static boolean preferDescendingTimeOrder(@Nullable List<OrderBy> preferredOrdering)
+ {
+ if (preferredOrdering != null && preferredOrdering.size() == 1) {
Review Comment:
ah we should probably allow if time is first too
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org