Skip to content

fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns #15633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
clintropolis merged 3 commits into from
Jan 8, 2024
Merged

fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns #15633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53da610
fix bugs with expression virtual column indexes for expression virtua…
clintropolis Jan 6, 2024
09cf355
more test
clintropolis Jan 6, 2024
0f3ce8a
fix compile
clintropolis Jan 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions benchmarks/src/test/java/org/apache/druid/benchmark/MockColumnIndexSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.druid.collections.bitmap.BitmapFactory;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.segment.column.ColumnIndexSupplier;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -58,6 +59,13 @@ public ColumnIndexSupplier getIndexSupplier(String column)
return indexSupplier;
}

@Nullable
@Override
public ColumnHolder getColumnHolder(String columnName)
{
return null;
}

@Nullable
@Override
public ColumnCapabilities getColumnCapabilities(String column)
Expand Down
17 changes: 7 additions & 10 deletions processing/src/main/java/org/apache/druid/math/expr/Expr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@
import org.apache.druid.java.util.common.ISE;
import org.apache.druid.math.expr.vector.ExprVectorProcessor;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.segment.column.ColumnIndexSupplier;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.index.semantic.DictionaryEncodedValueIndex;
Expand Down Expand Up @@ -194,28 +193,26 @@ default <T> ExprVectorProcessor<T> asVectorProcessor(VectorInputBindingInspector
}

@Nullable
default ColumnIndexSupplier asColumnIndexSupplier(ColumnSelector columnSelector, @Nullable ColumnType outputType)
default ColumnIndexSupplier asColumnIndexSupplier(
ColumnIndexSelector columnIndexSelector,
@Nullable ColumnType outputType
)
{
final Expr.BindingAnalysis details = analyzeInputs();
if (details.getRequiredBindings().size() == 1) {
// Single-column expression. We can use bitmap indexes if this column has an index and the expression can
// map over the values of the index.
final String column = Iterables.getOnlyElement(details.getRequiredBindings());

final ColumnHolder holder = columnSelector.getColumnHolder(column);
if (holder == null) {
// column doesn't exist, no index supplier
return null;
}
final ColumnCapabilities capabilities = holder.getCapabilities();
final ColumnIndexSupplier delegateIndexSupplier = holder.getIndexSupplier();
final ColumnIndexSupplier delegateIndexSupplier = columnIndexSelector.getIndexSupplier(column);
if (delegateIndexSupplier == null) {
return null;
}
final DictionaryEncodedValueIndex<?> delegateRawIndex = delegateIndexSupplier.as(
DictionaryEncodedValueIndex.class
);

final ColumnCapabilities capabilities = columnIndexSelector.getColumnCapabilities(column);
final ExpressionType inputType = ExpressionType.fromColumnTypeStrict(capabilities);
final ColumnType outType;
if (outputType == null) {
Expand Down
15 changes: 6 additions & 9 deletions processing/src/main/java/org/apache/druid/math/expr/IdentifierExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.math.expr.vector.ExprVectorProcessor;
import org.apache.druid.math.expr.vector.VectorProcessors;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.column.ColumnIndexSupplier;
import org.apache.druid.segment.column.ColumnType;

Expand Down Expand Up @@ -158,15 +157,13 @@ public ExprVectorProcessor<?> asVectorProcessor(VectorInputBindingInspector insp

@Nullable
@Override
public ColumnIndexSupplier asColumnIndexSupplier(ColumnSelector columnSelector, @Nullable ColumnType outputType)
public ColumnIndexSupplier asColumnIndexSupplier(
ColumnIndexSelector indexSelector,
@Nullable ColumnType outputType
)
{
// identifier just wraps a column, we can return its index supplier directly if the column exists
final ColumnHolder holder = columnSelector.getColumnHolder(binding);
if (holder == null) {
// column doesn't exist, no index supplier
return null;
}
return holder.getIndexSupplier();
return indexSelector.getIndexSupplier(binding);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions processing/src/main/java/org/apache/druid/query/filter/ColumnIndexSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@
package org.apache.druid.query.filter;

import org.apache.druid.collections.bitmap.BitmapFactory;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.segment.column.ColumnIndexSupplier;

import javax.annotation.Nullable;

/**
*/
public interface ColumnIndexSelector extends ColumnInspector
public interface ColumnIndexSelector extends ColumnSelector
{
int getNumRows();

Expand Down
7 changes: 0 additions & 7 deletions processing/src/main/java/org/apache/druid/segment/ColumnCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@

import javax.annotation.Nullable;
import java.util.HashMap;
import java.util.List;

public class ColumnCache implements ColumnSelector
{
Expand All @@ -45,12 +44,6 @@ public ColumnCache(QueryableIndex index, Closer closer)
}


@Override
public List<String> getColumnNames()
{
return index.getColumnNames();
}

@Nullable
@Override
public ColumnHolder getColumnHolder(String columnName)
Expand Down
7 changes: 0 additions & 7 deletions processing/src/main/java/org/apache/druid/segment/ColumnSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,11 @@
import org.apache.druid.segment.column.ColumnHolder;

import javax.annotation.Nullable;
import java.util.List;

/**
*/
public interface ColumnSelector extends ColumnInspector
{
/**
* This method is apparently no longer used anymore, so deprecating it.
*/
@Deprecated
List<String> getColumnNames();

@Nullable
ColumnHolder getColumnHolder(String columnName);

Expand Down
16 changes: 13 additions & 3 deletions processing/src/main/java/org/apache/druid/segment/ColumnSelectorColumnIndexSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public ColumnIndexSupplier getIndexSupplier(String column)
{
final ColumnIndexSupplier indexSupplier;
if (isVirtualColumn(column)) {
indexSupplier = virtualColumns.getIndexSupplier(column, columnSelector);
indexSupplier = virtualColumns.getIndexSupplier(column, this);
} else {
final ColumnHolder columnHolder = columnSelector.getColumnHolder(column);
// for missing columns we return null here. This allows callers to fabricate an 'all true' or 'all false'
Expand All @@ -90,9 +90,14 @@ public ColumnIndexSupplier getIndexSupplier(String column)
return indexSupplier;
}

private boolean isVirtualColumn(final String columnName)
@Nullable
@Override
public ColumnHolder getColumnHolder(String columnName)
{
return virtualColumns.getVirtualColumn(columnName) != null;
if (isVirtualColumn(columnName)) {
return null;
}
return columnSelector.getColumnHolder(columnName);
}

@Nullable
Expand All @@ -101,4 +106,9 @@ public ColumnCapabilities getColumnCapabilities(String column)
{
return virtualColumns.getColumnCapabilitiesWithFallback(columnSelector, column);
}

private boolean isVirtualColumn(final String columnName)
{
return virtualColumns.getVirtualColumn(columnName) != null;
}
}
7 changes: 0 additions & 7 deletions ...essing/src/main/java/org/apache/druid/segment/DeprecatedQueryableIndexColumnSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.druid.segment.column.ColumnHolder;

import javax.annotation.Nullable;
import java.util.List;

/**
* It likely looks weird that we are creating a new instance of ColumnSelector here that begins its life deprecated
Expand Down Expand Up @@ -51,12 +50,6 @@ public DeprecatedQueryableIndexColumnSelector(QueryableIndex index)
this.index = index;
}

@Override
public List<String> getColumnNames()
{
return index.getColumnNames();
}

@Nullable
@Override
public ColumnHolder getColumnHolder(String columnName)
Expand Down
12 changes: 8 additions & 4 deletions processing/src/main/java/org/apache/druid/segment/VirtualColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import org.apache.druid.java.util.common.Cacheable;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.ColumnIndexSupplier;
import org.apache.druid.segment.data.ReadableOffset;
Expand Down Expand Up @@ -299,13 +300,16 @@ default ColumnCapabilities capabilities(ColumnInspector inspector, String column

/**
* Get the {@link ColumnIndexSupplier} for the specified virtual column, with the assistance of a
* {@link ColumnSelector} to allow reading things from segments. If the virtual column has no indexes, this method
* will return null, or may also return a non-null supplier whose methods may return null values - having a supplier
* is no guarantee that the column has indexes.
* {@link ColumnIndexSelector} to allow reading things from segments. If the virtual column has no indexes, this
* method will return null, or may also return a non-null supplier whose methods may return null values - having a
* supplier is no guarantee that the column has indexes.
*/
@SuppressWarnings("unused")
@Nullable
default ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
default ColumnIndexSupplier getIndexSupplier(
String columnName,

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'columnName' is never used.
Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added columnName parameter to this method originally just in case stuff was using the 'usesDotNotation', unsure if the column name would be useful in such a situation, though perhaps it isn't really necessary and could be removed...

Copy link
Copy Markdown
Contributor

@gianm gianm Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There hasn't been much use that comes up for dot notation other than the original map virtual column. The JSON stuff could have used it, but we ended up going the more standard JSON_VALUE way instead. IMO it's fine to say simplify this and say that dot-notation virtual columns don't get to supply indexes. I'm OK either way though really.

ColumnIndexSelector columnIndexSelector
)
{
return NoIndexesColumnIndexSupplier.getInstance();
}
Expand Down
8 changes: 6 additions & 2 deletions processing/src/main/java/org/apache/druid/segment/VirtualColumns.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.druid.query.Query;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.segment.column.ColumnIndexSupplier;
Expand Down Expand Up @@ -185,10 +186,13 @@ public VirtualColumn getVirtualColumn(String columnName)
* is no guarantee that the column has indexes.
*/
@Nullable
public ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
public ColumnIndexSupplier getIndexSupplier(
String columnName,
ColumnIndexSelector columnIndexSelector
)
{
final VirtualColumn virtualColumn = getVirtualColumnForSelector(columnName);
return virtualColumn.getIndexSupplier(columnName, columnSelector);
return virtualColumn.getIndexSupplier(columnName, columnIndexSelector);
}

/**
Expand Down
9 changes: 6 additions & 3 deletions processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
import org.apache.druid.math.expr.Parser;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.ColumnValueSelector;
import org.apache.druid.segment.DimensionSelector;
Expand Down Expand Up @@ -244,9 +244,12 @@ public VectorObjectSelector makeVectorObjectSelector(String columnName, VectorCo

@Nullable
@Override
public ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
public ColumnIndexSupplier getIndexSupplier(
String columnName,
ColumnIndexSelector columnIndexSelector
)
{
return getParsedExpression().get().asColumnIndexSupplier(columnSelector, outputType);
return getParsedExpression().get().asColumnIndexSupplier(columnIndexSelector, outputType);
}

@Override
Expand Down
9 changes: 6 additions & 3 deletions processing/src/main/java/org/apache/druid/segment/virtual/FallbackVirtualColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
import org.apache.druid.java.util.common.Pair;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.ColumnValueSelector;
import org.apache.druid.segment.DimensionSelector;
Expand Down Expand Up @@ -189,9 +189,12 @@ public boolean usesDotNotation()

@Nullable
@Override
public ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
public ColumnIndexSupplier getIndexSupplier(
String columnName,
ColumnIndexSelector indexSelector
)
{
final ColumnHolder columnHolder = columnSelector.getColumnHolder(columnToUse(columnSelector).getDimension());
final ColumnHolder columnHolder = indexSelector.getColumnHolder(columnToUse(indexSelector).getDimension());
if (columnHolder == null) {
return null;
}
Expand Down
17 changes: 7 additions & 10 deletions processing/src/main/java/org/apache/druid/segment/virtual/ListFilteredVirtualColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@
import org.apache.druid.query.dimension.DefaultDimensionSpec;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.dimension.ListFilteredDimensionSpec;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.query.filter.DruidObjectPredicate;
import org.apache.druid.query.filter.DruidPredicateFactory;
import org.apache.druid.query.ordering.StringComparators;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.ColumnValueSelector;
import org.apache.druid.segment.DimensionSelector;
Expand Down Expand Up @@ -183,7 +183,10 @@ public boolean usesDotNotation()

@Nullable
@Override
public ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
public ColumnIndexSupplier getIndexSupplier(
String columnName,
ColumnIndexSelector indexSelector
)
{
if (delegate.getExtractionFn() != null) {
return NoIndexesColumnIndexSupplier.getInstance();
Expand All @@ -194,13 +197,7 @@ public ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector co
@Override
public <T> T as(Class<T> clazz)
{

final ColumnHolder holder = columnSelector.getColumnHolder(delegate.getDimension());
if (holder == null) {
return null;
}

ColumnIndexSupplier indexSupplier = holder.getIndexSupplier();
ColumnIndexSupplier indexSupplier = indexSelector.getIndexSupplier(delegate.getDimension());
if (indexSupplier == null) {
return null;
}
Expand Down Expand Up @@ -229,7 +226,7 @@ public <T> T as(Class<T> clazz)
}

// someday maybe we can have a better way to get row count..
final ColumnHolder time = columnSelector.getColumnHolder(ColumnHolder.TIME_COLUMN_NAME);
final ColumnHolder time = indexSelector.getColumnHolder(ColumnHolder.TIME_COLUMN_NAME);
final int numRows = time.getLength();
final Supplier<ImmutableBitmap> nullValueBitmapSupplier = Suppliers.memoize(
() -> underlyingIndex.getBitmapFactory().complement(
Expand Down
5 changes: 3 additions & 2 deletions processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.extraction.ExtractionFn;
import org.apache.druid.query.filter.ColumnIndexSelector;
import org.apache.druid.query.filter.DruidPredicateFactory;
import org.apache.druid.query.filter.ValueMatcher;
import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
Expand Down Expand Up @@ -1158,10 +1159,10 @@ private void computeVectorsIfNeeded()
@Override
public ColumnIndexSupplier getIndexSupplier(
String columnName,
ColumnSelector selector
ColumnIndexSelector indexSelector
)
{
ColumnHolder holder = selector.getColumnHolder(this.columnName);
ColumnHolder holder = indexSelector.getColumnHolder(this.columnName);
if (holder == null) {
return null;
}
Expand Down
Loading