Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.druid.java.util.common.UOE;
import org.apache.druid.java.util.common.logger.Logger;
import org.apache.druid.query.PerSegmentQueryOptimizationContext;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

Expand Down Expand Up @@ -68,7 +69,7 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
/**
* Returns whether or not this aggregation class supports vectorization. The default implementation returns false.
*/
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

Expand Down Expand Up @@ -72,7 +73,7 @@ public Comparator getComparator()
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.column.ValueType;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorValueSelector;

Expand Down Expand Up @@ -79,8 +82,12 @@ protected VectorValueSelector vectorSelector(VectorColumnSelectorFactory columnS
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
if (fieldName != null) {
final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
return expression == null && (capabilities == null || ValueType.isNumeric(capabilities.getType()));
}
return expression == null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.druid.query.filter.IntervalDimFilter;
import org.apache.druid.query.filter.ValueMatcher;
import org.apache.druid.query.filter.vector.VectorValueMatcher;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;
Expand Down Expand Up @@ -98,7 +99,7 @@ public BufferAggregator factorizeBuffered(ColumnSelectorFactory columnSelectorFa
@Override
public VectorAggregator factorizeVector(VectorColumnSelectorFactory columnSelectorFactory)
{
Preconditions.checkState(canVectorize(), "Cannot vectorize");
Preconditions.checkState(canVectorize(columnSelectorFactory), "Cannot vectorize");
final VectorValueMatcher valueMatcher = filter.makeVectorMatcher(columnSelectorFactory);
return new FilteredVectorAggregator(
valueMatcher,
Expand All @@ -107,9 +108,9 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory columnSelect
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return delegate.canVectorize() && filter.canVectorizeMatcher();
return delegate.canVectorize(columnInspector) && filter.canVectorizeMatcher();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.segment.BaseFloatColumnValueSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorValueSelector;

Expand Down Expand Up @@ -78,12 +80,15 @@ protected VectorValueSelector vectorSelector(VectorColumnSelectorFactory columnS
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
if (fieldName != null) {
final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
return expression == null && (capabilities == null || capabilities.getType().isNumeric());
}
return expression == null;
}


@Override
protected VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.segment.BaseLongColumnValueSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorValueSelector;

Expand Down Expand Up @@ -87,8 +89,12 @@ protected VectorAggregator factorizeVector(
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
if (fieldName != null) {
final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
return expression == null && (capabilities == null || capabilities.getType().isNumeric());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Which tests validate these conditions? Similar comments for the other AggregatorFactories. I see DoubleMeanAggregator is being tested

Copy link
Copy Markdown
Contributor Author

@maytasm maytasm Jun 17, 2020

Choose a reason for hiding this comment

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

SchemaEvolutionTest and DoubleMeanAggregationTest with Vectorize.TRUE triggers the bug case (where the column is not numeric and cannot be vectorized). I have made these tests run in both Vectorize.TRUE and Vectorize.FALSE mode to verify this fix. There are some other existing tests that are happy path (where the column is numeric and can be vectorized)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Just read SchemaEvolutionTest more closely, looks like it covers LongSum, DoubleSum and Count. I didn;t see a test that covers FloatSumAggregatorFactory

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Added FloatSumAggregatorFactory

return expression == null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public final BufferAggregator factorizeBuffered(ColumnSelectorFactory columnSele
@Override
public final VectorAggregator factorizeVector(VectorColumnSelectorFactory columnSelectorFactory)
{
Preconditions.checkState(canVectorize(), "Cannot vectorize");
Preconditions.checkState(canVectorize(columnSelectorFactory), "Cannot vectorize");
VectorValueSelector selector = vectorSelector(columnSelectorFactory);
VectorAggregator aggregator = factorizeVector(columnSelectorFactory, selector);
return NullHandling.replaceWithDefault() ? aggregator : new NullableNumericVectorAggregator(aggregator, selector);
Expand Down Expand Up @@ -135,12 +135,11 @@ protected abstract BufferAggregator factorizeBuffered(
* @see BufferAggregator
*/
protected VectorAggregator factorizeVector(
// Not used by current aggregators, but here for parity with "factorizeBuffered".
@SuppressWarnings("unused") VectorColumnSelectorFactory columnSelectorFactory,
VectorColumnSelectorFactory columnSelectorFactory,
VectorValueSelector selector
)
{
if (!canVectorize()) {
if (!canVectorize(columnSelectorFactory)) {
throw new UnsupportedOperationException("Cannot vectorize");
} else {
throw new UnsupportedOperationException("canVectorize returned true but 'factorizeVector' is not implemented");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.druid.query.PerSegmentQueryOptimizationContext;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

Expand Down Expand Up @@ -70,9 +71,9 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory columnSelect
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return delegate.canVectorize();
return delegate.canVectorize(columnInspector);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.druid.query.aggregation.cardinality.HyperLogLogCollectorAggregateCombiner;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseObjectColumnValueSelector;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.NilColumnValueSelector;
import org.apache.druid.segment.column.ColumnCapabilities;
Expand Down Expand Up @@ -140,7 +141,7 @@ public VectorAggregator factorizeVector(final VectorColumnSelectorFactory select
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@
import org.apache.druid.query.aggregation.BufferAggregator;
import org.apache.druid.query.aggregation.VectorAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.column.ColumnCapabilities;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -106,9 +108,10 @@ public VectorAggregator factorizeVector(final VectorColumnSelectorFactory select
}

@Override
public boolean canVectorize()
public boolean canVectorize(ColumnInspector columnInspector)
{
return true;
final ColumnCapabilities capabilities = columnInspector.getColumnCapabilities(fieldName);
return capabilities == null || capabilities.getType().isNumeric();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import org.apache.druid.java.util.common.parsers.CloseableIterator;
import org.apache.druid.query.QueryConfig;
import org.apache.druid.query.aggregation.AggregatorAdapters;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.filter.Filter;
import org.apache.druid.query.groupby.GroupByQuery;
Expand Down Expand Up @@ -85,7 +84,7 @@ public static boolean canVectorize(

return GroupByQueryEngineV2.isAllSingleValueDims(adapter::getColumnCapabilities, query.getDimensions(), true)
&& query.getDimensions().stream().allMatch(DimensionSpec::canVectorize)
&& query.getAggregatorSpecs().stream().allMatch(AggregatorFactory::canVectorize)
&& query.getAggregatorSpecs().stream().allMatch(aggregatorFactory -> aggregatorFactory.canVectorize(adapter))
&& adapter.canVectorize(filter, query.getVirtualColumns(), false);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public Sequence<Result<TimeseriesResultValue>> process(final TimeseriesQuery que

final boolean doVectorize = queryConfigToUse.getVectorize().shouldVectorize(
adapter.canVectorize(filter, query.getVirtualColumns(), descending)
&& query.getAggregatorSpecs().stream().allMatch(AggregatorFactory::canVectorize)
&& query.getAggregatorSpecs().stream().allMatch(aggregatorFactory -> aggregatorFactory.canVectorize(adapter))
);

final Sequence<Result<TimeseriesResultValue>> result;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* 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.segment.column.ColumnCapabilities;

import javax.annotation.Nullable;

public interface ColumnInspector
{
/**
* Returns capabilities of a particular column.
*
* @param column column name
*
* @return capabilities, or null
*/
@Nullable
ColumnCapabilities getColumnCapabilities(String column);
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
* @see org.apache.druid.segment.vector.VectorColumnSelectorFactory, the vectorized version
*/
@PublicApi
public interface ColumnSelectorFactory
public interface ColumnSelectorFactory extends ColumnInspector
{
DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec);

Expand All @@ -50,6 +50,7 @@ public interface ColumnSelectorFactory
*
* @return capabilities, or null
*/
@Override
@Nullable
ColumnCapabilities getColumnCapabilities(String column);
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
/**
*/
@PublicApi
public interface StorageAdapter extends CursorFactory
public interface StorageAdapter extends CursorFactory, ColumnInspector
{
Interval getInterval();
Indexed<String> getAvailableDimensions();
Expand Down Expand Up @@ -62,6 +62,7 @@ public interface StorageAdapter extends CursorFactory
*
* @return capabilities, or null
*/
@Override
@Nullable
ColumnCapabilities getColumnCapabilities(String column);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.apache.druid.segment.vector;

import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.segment.ColumnInspector;
import org.apache.druid.segment.column.ColumnCapabilities;

import javax.annotation.Nullable;
Expand All @@ -29,7 +30,7 @@
*
* @see org.apache.druid.segment.ColumnSelectorFactory, the non-vectorized version.
*/
public interface VectorColumnSelectorFactory
public interface VectorColumnSelectorFactory extends ColumnInspector
{
/**
* Returns a {@link VectorSizeInspector} for the {@link VectorCursor} that generated this object.
Expand Down Expand Up @@ -72,6 +73,7 @@ default int getMaxVectorSize()
*
* @return capabilities, or null if the column doesn't exist.
*/
@Override
@Nullable
ColumnCapabilities getColumnCapabilities(String column);
}
Loading