This is an automated email from the ASF dual-hosted git repository.
gianm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new df71642af99 fix: EmptyCursorHolder#canVectorize must depend on the
CursorBuildSpec. (#20822)
df71642af99 is described below
commit df71642af99a559caec628a969b3b610ab83625e
Author: Gian Merlino <[email protected]>
AuthorDate: Mon Nov 2 21:41:31 2026 -0800
fix: EmptyCursorHolder#canVectorize must depend on the CursorBuildSpec.
(#20822)
Previously, EmptyCursorHolder#canVectorize always returned true. However,
the expectation for CursorHolders is that they should check the
virtual columns and aggregators from the spec. Not doing so led to
the vector path being taken for virtual columns and aggregators that
are not able to vectorize.
---
.../apache/druid/segment/ConcatenatingCursor.java | 5 +-
.../apache/druid/segment/EmptyCursorHolder.java | 64 +++++++---------
.../PartialQueryableIndexCursorFactory.java | 4 +-
.../druid/segment/QueryableIndexCursorFactory.java | 2 +-
.../incremental/IncrementalIndexCursorFactory.java | 10 +--
.../segment/vector/ConcatenatingVectorCursor.java | 5 +-
.../druid/segment/EmptyCursorHolderTest.java | 86 ++++++++++++++++++----
7 files changed, 111 insertions(+), 65 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/segment/ConcatenatingCursor.java
b/processing/src/main/java/org/apache/druid/segment/ConcatenatingCursor.java
index 010dd0ca1e7..7111087cf99 100644
--- a/processing/src/main/java/org/apache/druid/segment/ConcatenatingCursor.java
+++ b/processing/src/main/java/org/apache/druid/segment/ConcatenatingCursor.java
@@ -21,6 +21,7 @@ package org.apache.druid.segment;
import com.google.common.base.Supplier;
import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.projections.ClusteringColumnSelectorFactory;
import javax.annotation.Nullable;
@@ -109,7 +110,9 @@ public final class ConcatenatingCursor implements Cursor
currentCursor = null;
if (!delegateSet) {
wrapperFactory.setDelegate(
-
EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN).asCursor().getColumnSelectorFactory(),
+ EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN,
RowSignature.empty())
+ .asCursor()
+ .getColumnSelectorFactory(),
clusteringValuesByGroup.get(holderSuppliers.size() - 1)
);
delegateSet = true;
diff --git
a/processing/src/main/java/org/apache/druid/segment/EmptyCursorHolder.java
b/processing/src/main/java/org/apache/druid/segment/EmptyCursorHolder.java
index fb3baf28d4d..620063ed7ed 100644
--- a/processing/src/main/java/org/apache/druid/segment/EmptyCursorHolder.java
+++ b/processing/src/main/java/org/apache/druid/segment/EmptyCursorHolder.java
@@ -20,9 +20,7 @@
package org.apache.druid.segment;
import org.apache.druid.error.DruidException;
-import org.apache.druid.query.Order;
import org.apache.druid.query.OrderBy;
-import org.apache.druid.query.context.QueryContextParameters;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.join.filter.AllNullColumnSelectorFactory;
@@ -36,46 +34,31 @@ import org.apache.druid.segment.vector.VectorObjectSelector;
import org.apache.druid.segment.vector.VectorValueSelector;
import javax.annotation.Nullable;
-import java.util.Collections;
import java.util.List;
/**
- * A {@link CursorHolder} that yields no rows. Its cursors are always done,
but its selector factories hand out harmless
- * nil selectors and {@link #canVectorize()} reports true.
- * <p>
- * Useful wherever a segment can be proven to match no rows without scanning
it. Use {@link #forSpec(CursorBuildSpec)}
- * to produce an empty cursor matching the preferred ordering of the {@link
CursorBuildSpec}.
+ * A {@link CursorHolder} that yields no rows. Useful wherever a segment can
be proven to match no rows without
+ * needing to scan it.
*/
public final class EmptyCursorHolder implements CursorHolder
{
- private static final EmptyCursorHolder NO_ORDER = new
EmptyCursorHolder(Collections.emptyList());
- private static final EmptyCursorHolder TIME_ASCENDING = new
EmptyCursorHolder(Cursors.ascendingTimeOrder());
- private static final EmptyCursorHolder TIME_DESCENDING = new
EmptyCursorHolder(Cursors.descendingTimeOrder());
-
private static final ColumnSelectorFactory NIL_SELECTOR_FACTORY = new
AllNullColumnSelectorFactory();
/**
- * An empty holder that advertises the spec's preferred ordering. A cursor
with no rows trivially satisfies any
- * ordering, so it can honestly claim whatever the query asked for. The two
{@code __time} directions reuse cached
- * singletons (the common case); any other non-empty preferred ordering gets
a fresh instance, and an empty preferred
- * ordering yields the unordered shared instance.
+ * An empty holder that advertises the spec's preferred ordering.
*/
- public static EmptyCursorHolder forSpec(CursorBuildSpec spec)
+ public static EmptyCursorHolder forSpec(CursorBuildSpec spec,
ColumnInspector inspector)
{
- final List<OrderBy> preferredOrdering = spec.getPreferredOrdering();
- final Order timeOrder = Cursors.getTimeOrdering(preferredOrdering);
- return switch (timeOrder) {
- case ASCENDING -> TIME_ASCENDING;
- case DESCENDING -> TIME_DESCENDING;
- default -> preferredOrdering.isEmpty() ? NO_ORDER : new
EmptyCursorHolder(preferredOrdering);
- };
+ return new EmptyCursorHolder(spec, inspector);
}
- private final List<OrderBy> ordering;
+ private final CursorBuildSpec spec;
+ private final ColumnInspector inspector;
- private EmptyCursorHolder(List<OrderBy> ordering)
+ private EmptyCursorHolder(CursorBuildSpec spec, ColumnInspector inspector)
{
- this.ordering = ordering;
+ this.spec = spec;
+ this.inspector = inspector;
}
@Override
@@ -121,16 +104,18 @@ public final class EmptyCursorHolder implements
CursorHolder
@Override
public boolean canVectorize()
{
- return true;
+ return QueryableIndexCursorHolder.canVectorizeVirtualColumnsAndAggregators(
+ spec.getVirtualColumns(),
+ spec.getAggregators(),
+ spec.getQueryContext(),
+ inspector
+ );
}
@Override
public VectorCursor asVectorCursor()
{
- // The cursor produces no vectors (always done, current size 0), so the
max vector size it reports is never used
- // to read data — the inspector and the nil selectors built from it are
internally consistent at any size. We
- // report the default rather than the query's requested vector size so
this holder can stay a shared singleton.
- final ReadableVectorInspector inspector = new ReadableVectorInspector()
+ final ReadableVectorInspector vectorInspector = new
ReadableVectorInspector()
{
@Override
public int getId()
@@ -141,7 +126,7 @@ public final class EmptyCursorHolder implements CursorHolder
@Override
public int getMaxVectorSize()
{
- return QueryContextParameters.VECTOR_SIZE.getLiteralDefault();
+ return spec.getQueryContext().getVectorSize();
}
@Override
@@ -155,13 +140,13 @@ public final class EmptyCursorHolder implements
CursorHolder
@Override
public ReadableVectorInspector getReadableVectorInspector()
{
- return inspector;
+ return vectorInspector;
}
@Override
public SingleValueDimensionVectorSelector
makeSingleValueDimensionSelector(DimensionSpec dimensionSpec)
{
- return NilVectorSelector.create(inspector);
+ return NilVectorSelector.create(vectorInspector);
}
@Override
@@ -178,13 +163,13 @@ public final class EmptyCursorHolder implements
CursorHolder
@Override
public VectorValueSelector makeValueSelector(String column)
{
- return NilVectorSelector.create(inspector);
+ return NilVectorSelector.create(vectorInspector);
}
@Override
public VectorObjectSelector makeObjectSelector(String column)
{
- return NilVectorSelector.create(inspector);
+ return NilVectorSelector.create(vectorInspector);
}
@Nullable
@@ -221,7 +206,7 @@ public final class EmptyCursorHolder implements CursorHolder
@Override
public int getMaxVectorSize()
{
- return inspector.getMaxVectorSize();
+ return vectorInspector.getMaxVectorSize();
}
@Override
@@ -235,6 +220,7 @@ public final class EmptyCursorHolder implements CursorHolder
@Override
public List<OrderBy> getOrdering()
{
- return ordering;
+ // A cursor with no rows trivially satisfies any ordering, so it can claim
whatever the query asked for.
+ return spec.getPreferredOrdering();
}
}
diff --git
a/processing/src/main/java/org/apache/druid/segment/PartialQueryableIndexCursorFactory.java
b/processing/src/main/java/org/apache/druid/segment/PartialQueryableIndexCursorFactory.java
index 5e9e573d0e0..faab37ce673 100644
---
a/processing/src/main/java/org/apache/druid/segment/PartialQueryableIndexCursorFactory.java
+++
b/processing/src/main/java/org/apache/druid/segment/PartialQueryableIndexCursorFactory.java
@@ -125,7 +125,7 @@ public class PartialQueryableIndexCursorFactory implements
CursorFactory
// restricted: dispatch from the restricted plan rather than letting the
delegate match over the whole segment
final PartialQueryableIndex.CursorPrefetchPlan plan =
index.planCursorPrefetch(spec, servableBundles);
if (plan.bundles().isEmpty()) {
- return EmptyCursorHolder.forSpec(spec);
+ return EmptyCursorHolder.forSpec(spec, delegate);
}
if (plan.clusterGroupPlan() != null) {
return delegate.makeClusteredCursorHolder(spec, plan.clusterGroupPlan());
@@ -142,7 +142,7 @@ public class PartialQueryableIndexCursorFactory implements
CursorFactory
if (plan.bundles().isEmpty()) {
// Filter rules out every cluster group, or nothing servable can answer
the spec: no bundle to acquire, nothing
// to download.
- return AsyncCursorHolder.completed(EmptyCursorHolder.forSpec(spec));
+ return AsyncCursorHolder.completed(EmptyCursorHolder.forSpec(spec,
delegate));
}
if (plan.clusterGroupPlan() != null) {
// reuse the plan's cluster resolution
diff --git
a/processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorFactory.java
b/processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorFactory.java
index 6632e2f1bb7..cfc27801d6f 100644
---
a/processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorFactory.java
+++
b/processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorFactory.java
@@ -127,7 +127,7 @@ public class QueryableIndexCursorFactory implements
ResidentCursorFactory
public CursorHolder makeClusteredCursorHolder(CursorBuildSpec spec,
ClusterGroupQueryPlan plan)
{
if (plan.survivingGroups().isEmpty()) {
- return EmptyCursorHolder.forSpec(spec);
+ return EmptyCursorHolder.forSpec(spec, this);
}
if (plan.survivingGroups().size() == 1) {
diff --git
a/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactory.java
b/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactory.java
index e364fe546ed..40969abcb92 100644
---
a/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactory.java
+++
b/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactory.java
@@ -154,16 +154,16 @@ public class IncrementalIndexCursorFactory implements
ResidentCursorFactory
final ClusteredValueGroupsBaseTableSchema summary =
clusteredBaseTable.toMetadataSchema();
final ClusterGroupQueryPlan plan =
Projections.planClusterGroupQuery(summary.getClusterGroups(), spec);
final List<TableClusterGroupSpec> surviving = plan.survivingGroups();
+ // Capabilities come from the groups rather than the base index, since the
base index's dimensions never see values
+ // computed by the cluster spec's virtual columns. Snapshot them like the
per-group selector factories do.
+ final ColumnInspector allGroupsInspector =
+ column ->
ColumnCapabilitiesImpl.snapshot(clusteredBaseTable.getColumnCapabilities(column),
COERCE_LOGIC);
if (surviving.isEmpty()) {
- return EmptyCursorHolder.forSpec(spec);
+ return EmptyCursorHolder.forSpec(spec, allGroupsInspector);
}
final RowSignature clusteringColumns = summary.getClusteringColumns();
- // Capabilities come from the groups rather than the base index, since the
base index's dimensions never see values
- // computed by the cluster spec's virtual columns. Snapshot them like the
per-group selector factories do.
- final ColumnInspector allGroupsInspector =
- column ->
ColumnCapabilitiesImpl.snapshot(clusteredBaseTable.getColumnCapabilities(column),
COERCE_LOGIC);
final List<Object[]> clusteringValuesByGroup = new
ArrayList<>(surviving.size());
final List<Supplier<CursorHolder>> holderSuppliers = new
ArrayList<>(surviving.size());
final Closer closer = Closer.create();
diff --git
a/processing/src/main/java/org/apache/druid/segment/vector/ConcatenatingVectorCursor.java
b/processing/src/main/java/org/apache/druid/segment/vector/ConcatenatingVectorCursor.java
index cf71fb2be0d..17377fbd778 100644
---
a/processing/src/main/java/org/apache/druid/segment/vector/ConcatenatingVectorCursor.java
+++
b/processing/src/main/java/org/apache/druid/segment/vector/ConcatenatingVectorCursor.java
@@ -24,6 +24,7 @@ import org.apache.druid.error.DruidException;
import org.apache.druid.segment.CursorBuildSpec;
import org.apache.druid.segment.CursorHolder;
import org.apache.druid.segment.EmptyCursorHolder;
+import org.apache.druid.segment.column.RowSignature;
import
org.apache.druid.segment.projections.ClusteringVectorColumnSelectorFactory;
import java.util.List;
@@ -116,7 +117,9 @@ public final class ConcatenatingVectorCursor implements
VectorCursor
currentCursor = null;
if (!delegateSet) {
wrapperFactory.setDelegate(
-
EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN).asVectorCursor().getColumnSelectorFactory(),
+ EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN,
RowSignature.empty())
+ .asVectorCursor()
+ .getColumnSelectorFactory(),
clusteringValuesByGroup.get(holderSuppliers.size() - 1)
);
delegateSet = true;
diff --git
a/processing/src/test/java/org/apache/druid/segment/EmptyCursorHolderTest.java
b/processing/src/test/java/org/apache/druid/segment/EmptyCursorHolderTest.java
index 7932cadcca8..e53d9f7eeb4 100644
---
a/processing/src/test/java/org/apache/druid/segment/EmptyCursorHolderTest.java
+++
b/processing/src/test/java/org/apache/druid/segment/EmptyCursorHolderTest.java
@@ -20,13 +20,27 @@
package org.apache.druid.segment;
import org.apache.druid.query.OrderBy;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.query.aggregation.CountAggregatorFactory;
+import org.apache.druid.query.aggregation.HistogramAggregatorFactory;
+import org.apache.druid.query.aggregation.LongSumAggregatorFactory;
+import org.apache.druid.query.context.QueryContextParameters;
+import org.apache.druid.query.expression.TestExprMacroTable;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import java.util.List;
+import java.util.Map;
class EmptyCursorHolderTest
{
+ private static final RowSignature INSPECTOR =
RowSignature.builder().add("x", ColumnType.LONG).build();
+ private static final VirtualColumns VECTORIZABLE_VIRTUAL_COLUMNS =
VirtualColumns.create(
+ new ExpressionVirtualColumn("v", "x + 1", ColumnType.LONG,
TestExprMacroTable.INSTANCE)
+ );
private static final CursorBuildSpec ASC = CursorBuildSpec.builder()
.setPreferredOrdering(Cursors.ascendingTimeOrder())
.build();
@@ -37,55 +51,95 @@ class EmptyCursorHolderTest
@Test
void testForTimeOrderAscendingAdvertisesAscendingTime()
{
- CursorHolder holder = EmptyCursorHolder.forSpec(ASC);
+ CursorHolder holder = EmptyCursorHolder.forSpec(ASC, INSPECTOR);
Assertions.assertEquals(
Cursors.ascendingTimeOrder(),
holder.getOrdering()
);
- Assertions.assertSame(EmptyCursorHolder.forSpec(ASC), holder);
}
@Test
void testForTimeOrderDescendingAdvertisesDescendingTime()
{
- CursorHolder holder = EmptyCursorHolder.forSpec(DESC);
+ CursorHolder holder = EmptyCursorHolder.forSpec(DESC, INSPECTOR);
Assertions.assertEquals(
Cursors.descendingTimeOrder(),
holder.getOrdering()
);
- Assertions.assertSame(EmptyCursorHolder.forSpec(DESC), holder);
}
@Test
- void testForTimeOrderNoneReturnsUnorderedInstance()
+ void testForTimeOrderNoneAdvertisesNoOrdering()
{
- CursorHolder holder = EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN);
+ CursorHolder holder = EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN,
INSPECTOR);
Assertions.assertEquals(
List.of(),
holder.getOrdering()
);
-
Assertions.assertSame(EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN),
holder);
}
@Test
void testForSpecNonTimeOrderingAdvertisedVerbatim()
{
- // preferredOrdering decouples from assumed __time ordering; an empty
cursor trivially satisfies any ordering, so
- // forSpec advertises a non-__time preferred ordering verbatim via a
fresh, uncached instance.
+ // An empty cursor trivially satisfies any ordering, so a non-__time
preferred ordering is advertised verbatim.
final List<OrderBy> ordering = List.of(OrderBy.ascending("dim"));
final CursorBuildSpec spec =
CursorBuildSpec.builder().setPreferredOrdering(ordering).build();
- final CursorHolder holder = EmptyCursorHolder.forSpec(spec);
+ final CursorHolder holder = EmptyCursorHolder.forSpec(spec, INSPECTOR);
Assertions.assertEquals(ordering, holder.getOrdering());
- // Not the unordered singleton, and arbitrary orderings are not cached (a
fresh instance each call).
- Assertions.assertNotSame(holder,
EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN));
- Assertions.assertNotSame(holder, EmptyCursorHolder.forSpec(spec));
}
@Test
void testCursorIsAlwaysDone()
{
- Assertions.assertTrue(EmptyCursorHolder.forSpec(ASC).asCursor().isDone());
- Assertions.assertTrue(EmptyCursorHolder.forSpec(DESC).asCursor().isDone());
-
Assertions.assertTrue(EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN).asCursor().isDone());
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(ASC,
INSPECTOR).asCursor().isDone());
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(DESC,
INSPECTOR).asCursor().isDone());
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN,
INSPECTOR).asCursor().isDone());
+ }
+
+ @Test
+ void testCanVectorizeWithoutVirtualColumnsOrAggregators()
+ {
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(CursorBuildSpec.FULL_SCAN,
INSPECTOR).canVectorize());
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(ASC,
INSPECTOR).canVectorize());
+ Assertions.assertTrue(EmptyCursorHolder.forSpec(DESC,
INSPECTOR).canVectorize());
+ }
+
+ @Test
+ void testCanVectorizeWithVectorizableVirtualColumnsAndAggregators()
+ {
+ final CursorBuildSpec spec =
+ CursorBuildSpec.builder(ASC)
+ .setVirtualColumns(VECTORIZABLE_VIRTUAL_COLUMNS)
+ .setAggregators(List.of(new
CountAggregatorFactory("c"), new LongSumAggregatorFactory("s", "v")))
+ .build();
+ final CursorHolder holder = EmptyCursorHolder.forSpec(spec, INSPECTOR);
+ Assertions.assertTrue(holder.canVectorize());
+ Assertions.assertEquals(Cursors.ascendingTimeOrder(),
holder.getOrdering());
+ }
+
+ @Test
+ void testCannotVectorizeWithNonVectorizableAggregator()
+ {
+ final CursorBuildSpec spec =
+ CursorBuildSpec.builder(DESC)
+ .setAggregators(List.of(new
HistogramAggregatorFactory("h", "x", List.of(1f))))
+ .build();
+ final CursorHolder holder = EmptyCursorHolder.forSpec(spec, INSPECTOR);
+ Assertions.assertFalse(holder.canVectorize());
+ Assertions.assertEquals(Cursors.descendingTimeOrder(),
holder.getOrdering());
+ Assertions.assertTrue(holder.asCursor().isDone());
+ }
+
+ @Test
+ void testCannotVectorizeWhenVirtualColumnVectorizationDisabled()
+ {
+ final CursorBuildSpec spec =
+ CursorBuildSpec.builder()
+ .setVirtualColumns(VECTORIZABLE_VIRTUAL_COLUMNS)
+ .setQueryContext(
+
QueryContext.of(Map.of(QueryContextParameters.VECTORIZE_VIRTUAL_COLUMNS.getName(),
"false"))
+ )
+ .build();
+ Assertions.assertFalse(EmptyCursorHolder.forSpec(spec,
INSPECTOR).canVectorize());
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
