From 59488e0ce6b38770673ea786d39e70d8ed9ce63b Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 5 Feb 2020 15:24:00 -0800 Subject: [PATCH 1/9] ANY Aggregator should not skip null values implementation --- .../query/aggregation/FloatSumAggregator.java | 2 +- .../query/aggregation/LongSumAggregator.java | 2 +- .../aggregation/any/DoubleAnyAggregator.java | 37 ++-- .../any/DoubleAnyAggregatorFactory.java | 170 ++++++++++++++---- .../any/DoubleAnyBufferAggregator.java | 58 ++---- .../aggregation/any/FloatAnyAggregator.java | 37 ++-- .../any/FloatAnyAggregatorFactory.java | 169 ++++++++++++++--- .../any/FloatAnyBufferAggregator.java | 55 +++--- .../aggregation/any/LongAnyAggregator.java | 35 ++-- .../any/LongAnyAggregatorFactory.java | 170 +++++++++++++++--- .../any/LongAnyBufferAggregator.java | 55 ++---- .../aggregation/any/NumericAnyAggregator.java | 68 +++++++ .../any/NumericAnyBufferAggregator.java | 103 +++++++++++ 13 files changed, 689 insertions(+), 272 deletions(-) create mode 100644 processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyAggregator.java create mode 100644 processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java index 90a2fd4fa760..caa5a26c46e6 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java @@ -28,7 +28,7 @@ */ public class FloatSumAggregator implements Aggregator { - static final Comparator COMPARATOR = new Ordering() + public static final Comparator COMPARATOR = new Ordering() { @Override public int compare(Object o, Object o1) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java index 30b339337d16..f9ae93c9d6ae 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java @@ -29,7 +29,7 @@ */ public class LongSumAggregator implements Aggregator { - static final Comparator COMPARATOR = new Ordering() + public static final Comparator COMPARATOR = new Ordering() { @Override public int compare(Object o, Object o1) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java index fc04c6d09576..c3d307a0d2b0 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java @@ -19,44 +19,35 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.query.aggregation.Aggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; import org.apache.druid.segment.BaseDoubleColumnValueSelector; +import javax.annotation.Nullable; + /** - * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class DoubleAnyAggregator implements Aggregator +public class DoubleAnyAggregator extends NumericAnyAggregator { - private final BaseDoubleColumnValueSelector valueSelector; - - private double foundValue; - private boolean isFound; + private double foundValue; public DoubleAnyAggregator(BaseDoubleColumnValueSelector valueSelector) { - this.valueSelector = valueSelector; + super(valueSelector); this.foundValue = 0; - this.isFound = false; } @Override - public void aggregate() + void setFoundValue() { - if (!isFound) { - foundValue = valueSelector.getDouble(); - isFound = true; - } + foundValue = valueSelector.getDouble(); } @Override + @Nullable public Object get() { - return foundValue; + return isNull ? null : foundValue; } @Override @@ -76,10 +67,4 @@ public double getDouble() { return foundValue; } - - @Override - public void close() - { - // no-op - } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java index f7714302e955..1a1b5c8dc3ce 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java @@ -19,79 +19,163 @@ package org.apache.druid.query.aggregation.any; -import com.fasterxml.jackson.annotation.JacksonInject; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import org.apache.druid.math.expr.ExprMacroTable; +import com.google.common.base.Preconditions; +import org.apache.druid.java.util.common.UOE; +import org.apache.druid.query.aggregation.AggregateCombiner; import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.SimpleDoubleAggregatorFactory; +import org.apache.druid.query.aggregation.DoubleSumAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseDoubleColumnValueSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.segment.NilColumnValueSelector; +import org.apache.druid.segment.column.ColumnHolder; import javax.annotation.Nullable; +import java.nio.ByteBuffer; import java.util.Collections; +import java.util.Comparator; import java.util.List; +import java.util.Objects; -public class DoubleAnyAggregatorFactory extends SimpleDoubleAggregatorFactory +public class DoubleAnyAggregatorFactory extends AggregatorFactory { + private static final Aggregator NIL_AGGREGATOR = new DoubleAnyAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate() + { + // no-op + } + }; + + private static final BufferAggregator NIL_BUFFER_AGGREGATOR = new DoubleAnyBufferAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate(ByteBuffer buf, int position) + { + // no-op + } + }; + + private final String fieldName; + private final String name; + private final boolean storeDoubleAsFloat; + @JsonCreator public DoubleAnyAggregatorFactory( @JsonProperty("name") String name, - @JsonProperty("fieldName") final String fieldName, - @JsonProperty("expression") @Nullable String expression, - @JacksonInject ExprMacroTable macroTable + @JsonProperty("fieldName") final String fieldName ) { - super(macroTable, name, fieldName, expression); - } + Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); + Preconditions.checkNotNull(fieldName, "Must have a valid, non-null fieldName"); - public DoubleAnyAggregatorFactory(String name, String fieldName) - { - this(name, fieldName, null, ExprMacroTable.nil()); + this.name = name; + this.fieldName = fieldName; + this.storeDoubleAsFloat = ColumnHolder.storeDoubleAsFloat(); } @Override - protected double nullValue() + public Aggregator factorize(ColumnSelectorFactory metricFactory) { - return Double.NaN; + final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_AGGREGATOR; + } else { + return new DoubleAnyAggregator( + valueSelector + ); + } } @Override - protected Aggregator buildAggregator(BaseDoubleColumnValueSelector selector) + public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) { - return new DoubleAnyAggregator(selector); + final BaseDoubleColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_BUFFER_AGGREGATOR; + } else { + return new DoubleAnyBufferAggregator( + valueSelector + ); + } } @Override - protected BufferAggregator buildBufferAggregator(BaseDoubleColumnValueSelector selector) + public Comparator getComparator() { - return new DoubleAnyBufferAggregator(selector); + //TODO check handle null + return DoubleSumAggregator.COMPARATOR; } @Override @Nullable public Object combine(@Nullable Object lhs, @Nullable Object rhs) { - if (lhs != null) { - return lhs; - } else { - return rhs; - } + return lhs; + } + + @Override + public AggregateCombiner makeAggregateCombiner() + { + throw new UOE("DoubleAnyAggregatorFactory is not supported during ingestion for rollup"); } @Override public AggregatorFactory getCombiningFactory() { - return new DoubleAnyAggregatorFactory(name, name, null, macroTable); + return new DoubleAnyAggregatorFactory(name, name); } @Override public List getRequiredColumns() { - return Collections.singletonList(new DoubleAnyAggregatorFactory(fieldName, fieldName, expression, macroTable)); + return Collections.singletonList(new DoubleAnyAggregatorFactory(fieldName, fieldName)); + } + + @Override + public Object deserialize(Object object) + { + // handle "NaN" / "Infinity" values serialized as strings in JSON + if (object instanceof String) { + return Double.parseDouble((String) object); + } + return object; + } + + @Override + @Nullable + public Object finalizeComputation(@Nullable Object object) + { + return object; + } + + @Override + @JsonProperty + public String getName() + { + return name; + } + + @JsonProperty + public String getFieldName() + { + return fieldName; + } + + @Override + public List requiredFields() + { + return Collections.singletonList(fieldName); } @Override @@ -99,23 +183,49 @@ public byte[] getCacheKey() { return new CacheKeyBuilder(AggregatorUtil.DOUBLE_ANY_CACHE_TYPE_ID) .appendString(fieldName) - .appendString(expression) .build(); } + @Override + public String getTypeName() + { + // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde + return storeDoubleAsFloat ? "float" : "double"; + } + @Override public int getMaxIntermediateSize() { - return Double.BYTES + Byte.BYTES; + return Double.BYTES + Byte.BYTES + Byte.BYTES; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + DoubleAnyAggregatorFactory that = (DoubleAnyAggregatorFactory) o; + + return fieldName.equals(that.fieldName) && name.equals(that.name); + } + + @Override + public int hashCode() + { + return Objects.hash(fieldName, name); } @Override public String toString() { return "DoubleAnyAggregatorFactory{" + - "fieldName='" + fieldName + '\'' + - ", expression='" + expression + '\'' + - ", name='" + name + '\'' + + "name='" + name + '\'' + + ", fieldName='" + fieldName + '\'' + '}'; } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java index 022f9ce84fbc..df77f5458f22 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java @@ -19,81 +19,59 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.common.config.NullHandling; -import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; -import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; import org.apache.druid.segment.BaseDoubleColumnValueSelector; +import javax.annotation.Nullable; import java.nio.ByteBuffer; /** - * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class DoubleAnyBufferAggregator implements BufferAggregator +public class DoubleAnyBufferAggregator extends NumericAnyBufferAggregator { - private static final byte BYTE_FLAG_IS_NOT_SET = 0; - private static final byte BYTE_FLAG_IS_SET = 1; - private final BaseDoubleColumnValueSelector valueSelector; - - public DoubleAnyBufferAggregator(BaseDoubleColumnValueSelector valueSelector) + public DoubleAnyBufferAggregator( + BaseDoubleColumnValueSelector valueSelector + ) { - this.valueSelector = valueSelector; + super(valueSelector); } @Override - public void init(ByteBuffer buf, int position) + void initValue(ByteBuffer buf, int position) { - buf.put(position, BYTE_FLAG_IS_NOT_SET); - buf.putDouble(position + Byte.BYTES, NullHandling.ZERO_DOUBLE); + buf.putDouble(getFoundValueStoredPosition(position), 0); } @Override - public void aggregate(ByteBuffer buf, int position) + void putValue(ByteBuffer buf, int position) { - if (buf.get(position) == BYTE_FLAG_IS_NOT_SET) { - buf.put(position, BYTE_FLAG_IS_SET); - buf.putDouble(position + Byte.BYTES, valueSelector.getDouble()); - } + buf.putDouble(getFoundValueStoredPosition(position), valueSelector.getDouble()); } @Override + @Nullable public Object get(ByteBuffer buf, int position) { - return buf.getDouble(position + Byte.BYTES); + final boolean isNull = isValueNull(buf, position); + return isNull ? null : buf.getDouble(getFoundValueStoredPosition(position)); } @Override public float getFloat(ByteBuffer buf, int position) { - return (float) buf.getDouble(position + Byte.BYTES); + return (float) buf.getDouble(getFoundValueStoredPosition(position)); } @Override public long getLong(ByteBuffer buf, int position) { - return (long) buf.getDouble(position + Byte.BYTES); + return (long) buf.getDouble(getFoundValueStoredPosition(position)); } @Override public double getDouble(ByteBuffer buf, int position) { - return buf.getDouble(position + Byte.BYTES); - } - - @Override - public void close() - { - // no-op - } - - @Override - public void inspectRuntimeShape(RuntimeShapeInspector inspector) - { - inspector.visit("valueSelector", valueSelector); + return buf.getDouble(getFoundValueStoredPosition(position)); } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java index 26976cfec377..a13a01be133b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java @@ -19,44 +19,35 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.query.aggregation.Aggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; import org.apache.druid.segment.BaseFloatColumnValueSelector; +import javax.annotation.Nullable; + /** - * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class FloatAnyAggregator implements Aggregator +public class FloatAnyAggregator extends NumericAnyAggregator { - private final BaseFloatColumnValueSelector valueSelector; - - private float foundValue; - private boolean isFound; + private float foundValue; public FloatAnyAggregator(BaseFloatColumnValueSelector valueSelector) { - this.valueSelector = valueSelector; + super(valueSelector); this.foundValue = 0; - this.isFound = false; } @Override - public void aggregate() + void setFoundValue() { - if (!isFound) { - foundValue = valueSelector.getFloat(); - isFound = true; - } + foundValue = valueSelector.getFloat(); } @Override + @Nullable public Object get() { - return foundValue; + return isNull ? null : foundValue; } @Override @@ -76,10 +67,4 @@ public double getDouble() { return (double) foundValue; } - - @Override - public void close() - { - // no-op - } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java index 932d7434de93..3d820560fdd8 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java @@ -22,76 +22,165 @@ import com.fasterxml.jackson.annotation.JacksonInject; 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.UOE; import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.query.aggregation.AggregateCombiner; import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.FloatSumAggregator; import org.apache.druid.query.aggregation.SimpleFloatAggregatorFactory; +import org.apache.druid.query.aggregation.first.FloatFirstAggregator; +import org.apache.druid.query.aggregation.first.FloatFirstAggregatorFactory; +import org.apache.druid.query.aggregation.first.FloatFirstBufferAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseFloatColumnValueSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.segment.NilColumnValueSelector; +import org.apache.druid.segment.column.ColumnHolder; import javax.annotation.Nullable; +import java.nio.ByteBuffer; +import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.List; +import java.util.Objects; -public class FloatAnyAggregatorFactory extends SimpleFloatAggregatorFactory +public class FloatAnyAggregatorFactory extends AggregatorFactory { + private static final Aggregator NIL_AGGREGATOR = new FloatAnyAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate() + { + // no-op + } + }; + + private static final BufferAggregator NIL_BUFFER_AGGREGATOR = new FloatAnyBufferAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate(ByteBuffer buf, int position) + { + // no-op + } + }; + + private final String fieldName; + private final String name; + @JsonCreator public FloatAnyAggregatorFactory( @JsonProperty("name") String name, - @JsonProperty("fieldName") final String fieldName, - @JsonProperty("expression") @Nullable String expression, - @JacksonInject ExprMacroTable macroTable + @JsonProperty("fieldName") final String fieldName ) { - super(macroTable, name, fieldName, expression); - } + Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); + Preconditions.checkNotNull(fieldName, "Must have a valid, non-null fieldName"); - public FloatAnyAggregatorFactory(String name, String fieldName) - { - this(name, fieldName, null, ExprMacroTable.nil()); + this.name = name; + this.fieldName = fieldName; } @Override - protected float nullValue() + public Aggregator factorize(ColumnSelectorFactory metricFactory) { - return Float.NaN; + final BaseFloatColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_AGGREGATOR; + } else { + return new FloatAnyAggregator( + valueSelector + ); + } } @Override - protected Aggregator buildAggregator(BaseFloatColumnValueSelector selector) + public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) { - return new FloatAnyAggregator(selector); + final BaseFloatColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_BUFFER_AGGREGATOR; + } else { + return new FloatAnyBufferAggregator( + valueSelector + ); + } } @Override - protected BufferAggregator buildBufferAggregator(BaseFloatColumnValueSelector selector) + public Comparator getComparator() { - return new FloatAnyBufferAggregator(selector); + return FloatSumAggregator.COMPARATOR; } @Override @Nullable public Object combine(@Nullable Object lhs, @Nullable Object rhs) { - if (lhs != null) { - return lhs; - } else { - return rhs; - } + return lhs; + } + + @Override + public AggregateCombiner makeAggregateCombiner() + { + throw new UOE("FloatAnyAggregatorFactory is not supported during ingestion for rollup"); } + @Override public AggregatorFactory getCombiningFactory() { - return new FloatAnyAggregatorFactory(name, name, null, macroTable); + return new FloatAnyAggregatorFactory(name, name); } @Override public List getRequiredColumns() { - return Collections.singletonList(new FloatAnyAggregatorFactory(fieldName, fieldName, expression, macroTable)); + return Collections.singletonList(new FloatAnyAggregatorFactory(fieldName, fieldName)); + } + + @Override + public Object deserialize(Object object) + { + // handle "NaN" / "Infinity" values serialized as strings in JSON + if (object instanceof String) { + return Float.parseFloat((String) object); + } + return object; + } + + @Override + @Nullable + public Object finalizeComputation(@Nullable Object object) + { + return object; + } + + @Override + @JsonProperty + public String getName() + { + return name; + } + + @JsonProperty + public String getFieldName() + { + return fieldName; + } + + @Override + public List requiredFields() + { + return Collections.singletonList(fieldName); } @Override @@ -99,23 +188,49 @@ public byte[] getCacheKey() { return new CacheKeyBuilder(AggregatorUtil.FLOAT_ANY_CACHE_TYPE_ID) .appendString(fieldName) - .appendString(expression) .build(); } + @Override + public String getTypeName() + { + // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde + return "float"; + } + @Override public int getMaxIntermediateSize() { - return Float.BYTES + Byte.BYTES; + return Float.BYTES + Byte.BYTES + Byte.BYTES; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + FloatAnyAggregatorFactory that = (FloatAnyAggregatorFactory) o; + + return fieldName.equals(that.fieldName) && name.equals(that.name); + } + + @Override + public int hashCode() + { + return Objects.hash(fieldName, name); } @Override public String toString() { return "FloatAnyAggregatorFactory{" + - "fieldName='" + fieldName + '\'' + - ", expression='" + expression + '\'' + - ", name='" + name + '\'' + + "name='" + name + '\'' + + ", fieldName='" + fieldName + '\'' + '}'; } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java index bb6b6691e936..cb90c63c1ca8 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java @@ -23,77 +23,62 @@ import org.apache.druid.query.aggregation.NullableNumericAggregator; import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseFloatColumnValueSelector; +import javax.annotation.Nullable; import java.nio.ByteBuffer; /** - * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class FloatAnyBufferAggregator implements BufferAggregator +public class FloatAnyBufferAggregator extends NumericAnyBufferAggregator { - private static final byte BYTE_FLAG_IS_NOT_SET = 0; - private static final byte BYTE_FLAG_IS_SET = 1; - private static final float NULL_VALUE = 0; - private final BaseFloatColumnValueSelector valueSelector; - - public FloatAnyBufferAggregator(BaseFloatColumnValueSelector valueSelector) + public FloatAnyBufferAggregator( + BaseFloatColumnValueSelector valueSelector + ) { - this.valueSelector = valueSelector; + super(valueSelector); } + @Override - public void init(ByteBuffer buf, int position) + void initValue(ByteBuffer buf, int position) { - buf.put(position, BYTE_FLAG_IS_NOT_SET); - buf.putFloat(position + Byte.BYTES, NULL_VALUE); + buf.putFloat(getFoundValueStoredPosition(position), 0); } @Override - public void aggregate(ByteBuffer buf, int position) + void putValue(ByteBuffer buf, int position) { - if (buf.get(position) == BYTE_FLAG_IS_NOT_SET) { - buf.put(position, BYTE_FLAG_IS_SET); - buf.putFloat(position + Byte.BYTES, valueSelector.getFloat()); - } + buf.putFloat(getFoundValueStoredPosition(position), valueSelector.getFloat()); } @Override + @Nullable public Object get(ByteBuffer buf, int position) { - return buf.getFloat(position + Byte.BYTES); + final boolean isNull = isValueNull(buf, position); + return isNull ? null : buf.getFloat(getFoundValueStoredPosition(position)); } @Override public float getFloat(ByteBuffer buf, int position) { - return buf.getFloat(position + Byte.BYTES); + return buf.getFloat(getFoundValueStoredPosition(position)); } @Override public long getLong(ByteBuffer buf, int position) { - return (long) buf.getFloat(position + Byte.BYTES); + return (long) buf.getFloat(getFoundValueStoredPosition(position)); } @Override public double getDouble(ByteBuffer buf, int position) { - return (double) buf.getFloat(position + Byte.BYTES); + return (double) buf.getFloat(getFoundValueStoredPosition(position)); } - @Override - public void close() - { - // no-op - } - - @Override - public void inspectRuntimeShape(RuntimeShapeInspector inspector) - { - inspector.visit("valueSelector", valueSelector); - } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java index 3689564e2a9e..daeda110080e 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java @@ -22,41 +22,36 @@ import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.NullableNumericAggregator; import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; +import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseLongColumnValueSelector; +import javax.annotation.Nullable; + /** - * This Aggregator is created by the {@link LongAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link LongAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class LongAnyAggregator implements Aggregator +public class LongAnyAggregator extends NumericAnyAggregator { - private final BaseLongColumnValueSelector valueSelector; - - private long foundValue; - private boolean isFound; + private long foundValue; public LongAnyAggregator(BaseLongColumnValueSelector valueSelector) { - this.valueSelector = valueSelector; + super(valueSelector); this.foundValue = 0; - this.isFound = false; } @Override - public void aggregate() + void setFoundValue() { - if (!isFound) { - foundValue = valueSelector.getLong(); - isFound = true; - } + foundValue = valueSelector.getLong(); } @Override + @Nullable public Object get() { - return foundValue; + return isNull ? null : foundValue; } @Override @@ -76,10 +71,4 @@ public double getDouble() { return (double) foundValue; } - - @Override - public void close() - { - // no-op - } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java index 10e26275d5e5..0e5886c3d93b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java @@ -22,76 +22,163 @@ import com.fasterxml.jackson.annotation.JacksonInject; 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.UOE; import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.query.aggregation.AggregateCombiner; import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.LongSumAggregator; import org.apache.druid.query.aggregation.SimpleLongAggregatorFactory; +import org.apache.druid.query.aggregation.first.LongFirstAggregator; +import org.apache.druid.query.aggregation.first.LongFirstAggregatorFactory; +import org.apache.druid.query.aggregation.first.LongFirstBufferAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseLongColumnValueSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.segment.NilColumnValueSelector; +import org.apache.druid.segment.column.ColumnHolder; import javax.annotation.Nullable; +import java.nio.ByteBuffer; +import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.List; -public class LongAnyAggregatorFactory extends SimpleLongAggregatorFactory +public class LongAnyAggregatorFactory extends AggregatorFactory { + private static final Aggregator NIL_AGGREGATOR = new LongAnyAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate() + { + // no-op + } + }; + + private static final BufferAggregator NIL_BUFFER_AGGREGATOR = new LongAnyBufferAggregator( + NilColumnValueSelector.instance() + ) + { + @Override + public void aggregate(ByteBuffer buf, int position) + { + // no-op + } + }; + + private final String fieldName; + private final String name; + @JsonCreator public LongAnyAggregatorFactory( @JsonProperty("name") String name, - @JsonProperty("fieldName") final String fieldName, - @JsonProperty("expression") @Nullable String expression, - @JacksonInject ExprMacroTable macroTable + @JsonProperty("fieldName") final String fieldName ) { - super(macroTable, name, fieldName, expression); - } + Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); + Preconditions.checkNotNull(fieldName, "Must have a valid, non-null fieldName"); - public LongAnyAggregatorFactory(String name, String fieldName) - { - this(name, fieldName, null, ExprMacroTable.nil()); + this.name = name; + this.fieldName = fieldName; } @Override - protected long nullValue() + public Aggregator factorize(ColumnSelectorFactory metricFactory) { - return 0; + final BaseLongColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_AGGREGATOR; + } else { + return new LongAnyAggregator( + valueSelector + ); + } } @Override - protected Aggregator buildAggregator(BaseLongColumnValueSelector selector) + public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) { - return new LongAnyAggregator(selector); + final BaseLongColumnValueSelector valueSelector = metricFactory.makeColumnValueSelector(fieldName); + if (valueSelector instanceof NilColumnValueSelector) { + return NIL_BUFFER_AGGREGATOR; + } else { + return new LongAnyBufferAggregator( + valueSelector + ); + } } @Override - protected BufferAggregator buildBufferAggregator(BaseLongColumnValueSelector selector) + public Comparator getComparator() { - return new LongAnyBufferAggregator(selector); + return LongSumAggregator.COMPARATOR; } @Override @Nullable public Object combine(@Nullable Object lhs, @Nullable Object rhs) { - if (lhs != null) { - return lhs; - } else { - return rhs; - } + return lhs; + } + + @Override + public AggregateCombiner makeAggregateCombiner() + { + throw new UOE("LongAnyAggregatorFactory is not supported during ingestion for rollup"); } @Override public AggregatorFactory getCombiningFactory() { - return new LongAnyAggregatorFactory(name, name, null, macroTable); + return new LongAnyAggregatorFactory(name, name); } @Override public List getRequiredColumns() { - return Collections.singletonList(new LongAnyAggregatorFactory(fieldName, fieldName, expression, macroTable)); + return Collections.singletonList(new LongAnyAggregatorFactory(fieldName, fieldName)); + } + + @Override + public Object deserialize(Object object) + { + // handle "NaN" / "Infinity" values serialized as strings in JSON + if (object instanceof String) { + return Float.parseFloat((String) object); + } + return object; + } + + @Override + @Nullable + public Object finalizeComputation(@Nullable Object object) + { + return object; + } + + @Override + @JsonProperty + public String getName() + { + return name; + } + + @JsonProperty + public String getFieldName() + { + return fieldName; + } + + @Override + public List requiredFields() + { + return Collections.singletonList(fieldName); } @Override @@ -99,23 +186,52 @@ public byte[] getCacheKey() { return new CacheKeyBuilder(AggregatorUtil.LONG_ANY_CACHE_TYPE_ID) .appendString(fieldName) - .appendString(expression) .build(); } + @Override + public String getTypeName() + { + // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde + return "long"; + } + @Override public int getMaxIntermediateSize() { - return Long.BYTES + Byte.BYTES; + return Long.BYTES + Byte.BYTES + Byte.BYTES; + } + + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + LongAnyAggregatorFactory that = (LongAnyAggregatorFactory) o; + + return fieldName.equals(that.fieldName) && name.equals(that.name); + } + + @Override + public int hashCode() + { + int result = name.hashCode(); + result = 31 * result + fieldName.hashCode(); + return result; } @Override public String toString() { return "LongAnyAggregatorFactory{" + - "fieldName='" + fieldName + '\'' + - ", expression='" + expression + '\'' + - ", name='" + name + '\'' + + "name='" + name + '\'' + + ", fieldName='" + fieldName + '\'' + '}'; } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java index 00d12af1f0f6..c8f0b7a8085b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java @@ -23,77 +23,60 @@ import org.apache.druid.query.aggregation.NullableNumericAggregator; import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseLongColumnValueSelector; +import javax.annotation.Nullable; import java.nio.ByteBuffer; /** - * This Aggregator is created by the {@link LongAnyAggregatorFactory} which extends from - * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then {@link NullableNumericAggregatorFactory} - * will wrap this aggregator in {@link NullableNumericAggregator} and can handle all null in that class. - * Hence, no null will ever be pass into this aggregator from the valueSelector. + * This Aggregator is created by the {@link LongAnyAggregatorFactory} which has no special null handling logic. + * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. */ -public class LongAnyBufferAggregator implements BufferAggregator +public class LongAnyBufferAggregator extends NumericAnyBufferAggregator { - private static final byte BYTE_FLAG_IS_NOT_SET = 0; - private static final byte BYTE_FLAG_IS_SET = 1; - private static final long NULL_VALUE = 0; - private final BaseLongColumnValueSelector valueSelector; - - public LongAnyBufferAggregator(BaseLongColumnValueSelector valueSelector) + public LongAnyBufferAggregator( + BaseLongColumnValueSelector valueSelector + ) { - this.valueSelector = valueSelector; + super(valueSelector); } @Override - public void init(ByteBuffer buf, int position) + void initValue(ByteBuffer buf, int position) { - buf.put(position, BYTE_FLAG_IS_NOT_SET); - buf.putLong(position + Byte.BYTES, NULL_VALUE); + buf.putLong(getFoundValueStoredPosition(position), 0); } @Override - public void aggregate(ByteBuffer buf, int position) + void putValue(ByteBuffer buf, int position) { - if (buf.get(position) == BYTE_FLAG_IS_NOT_SET) { - buf.put(position, BYTE_FLAG_IS_SET); - buf.putLong(position + Byte.BYTES, valueSelector.getLong()); - } + buf.putLong(getFoundValueStoredPosition(position), valueSelector.getLong()); } @Override + @Nullable public Object get(ByteBuffer buf, int position) { - return buf.getLong(position + Byte.BYTES); + final boolean isNull = isValueNull(buf, position); + return isNull ? null : buf.getLong(getFoundValueStoredPosition(position)); } @Override public float getFloat(ByteBuffer buf, int position) { - return (float) buf.getLong(position + Byte.BYTES); + return (float) buf.getLong(getFoundValueStoredPosition(position)); } @Override public double getDouble(ByteBuffer buf, int position) { - return (double) buf.getLong(position + Byte.BYTES); + return (double) buf.getLong(getFoundValueStoredPosition(position)); } @Override public long getLong(ByteBuffer buf, int position) { - return buf.getLong(position + Byte.BYTES); - } - - @Override - public void close() - { - // no-op - } - - @Override - public void inspectRuntimeShape(RuntimeShapeInspector inspector) - { - inspector.visit("valueSelector", valueSelector); + return buf.getLong(getFoundValueStoredPosition(position)); } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyAggregator.java new file mode 100644 index 000000000000..25102e61c59b --- /dev/null +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyAggregator.java @@ -0,0 +1,68 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.query.aggregation.Aggregator; +import org.apache.druid.segment.BaseNullableColumnValueSelector; + +/** + * Base type for on heap 'any' aggregator for primitive numeric column selectors + */ +public abstract class NumericAnyAggregator implements Aggregator +{ + private final boolean useDefault = NullHandling.replaceWithDefault(); + final TSelector valueSelector; + + boolean isNull; + boolean isFound; + + public NumericAnyAggregator(TSelector valueSelector) + { + this.valueSelector = valueSelector; + this.isNull = !useDefault; + this.isFound = false; + } + + /** + * Store the found primitive value + */ + abstract void setFoundValue(); + + @Override + public void aggregate() + { + if (!isFound) { + if (useDefault || !valueSelector.isNull()) { + setFoundValue(); + isNull = false; + } else { + isNull = true; + } + isFound = true; + } + } + + @Override + public void close() + { + // nothing to close + } +} diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java new file mode 100644 index 000000000000..f4a064464d58 --- /dev/null +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java @@ -0,0 +1,103 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import org.apache.druid.segment.BaseNullableColumnValueSelector; + +import java.nio.ByteBuffer; + +/** + * Base type for buffer based 'any' aggregator for primitive numeric column selectors + */ +public abstract class NumericAnyBufferAggregator + implements BufferAggregator +{ + private static final byte BYTE_FLAG_IS_NOT_SET = 0; + private static final byte BYTE_FLAG_IS_SET = 1; + private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0; + private static final int IS_NULL_FLAG_OFFSET_POSITION = IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES; + private static final int FOUND_VALUE_OFFSET_POSITION = IS_NULL_FLAG_OFFSET_POSITION + Byte.BYTES; + + private final boolean useDefault = NullHandling.replaceWithDefault(); + + final TSelector valueSelector; + + public NumericAnyBufferAggregator(TSelector valueSelector) + { + this.valueSelector = valueSelector; + } + + /** + * Initialize the buffer value given the initial offset position within the byte buffer for initialization + */ + abstract void initValue(ByteBuffer buf, int position); + + /** + * Place the primitive value in the buffer given the initial offset position within the byte buffer + * at which the current aggregate value is stored + */ + abstract void putValue(ByteBuffer buf, int position); + + @Override + public void init(ByteBuffer buf, int position) + { + buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); + buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, useDefault ? BYTE_FLAG_IS_NOT_SET : BYTE_FLAG_IS_SET); + initValue(buf, position); + } + + @Override + public void aggregate(ByteBuffer buf, int position) + { + if (buf.get(position + IS_FOUND_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_NOT_SET) { + if (useDefault || !valueSelector.isNull()) { + putValue(buf, position); + buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); + } else { + buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); + } + buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); + } + } + + boolean isValueNull(ByteBuffer buf, int position) + { + return buf.get(position + IS_NULL_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_SET; + } + + int getFoundValueStoredPosition(int position) { + return position + FOUND_VALUE_OFFSET_POSITION; + } + + @Override + public void close() + { + // no resources to cleanup + } + + @Override + public void inspectRuntimeShape(RuntimeShapeInspector inspector) + { + inspector.visit("valueSelector", valueSelector); + } +} From 4344b4a6020f7f0c9e5416a32e6cb31ee8d5feb4 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 6 Feb 2020 13:23:13 -0800 Subject: [PATCH 2/9] add tests --- .../query/aggregation/FloatSumAggregator.java | 2 +- .../query/aggregation/LongSumAggregator.java | 2 +- .../aggregation/any/DoubleAnyAggregator.java | 2 +- .../any/DoubleAnyAggregatorFactory.java | 5 +- .../aggregation/any/FloatAnyAggregator.java | 2 +- .../any/FloatAnyAggregatorFactory.java | 4 +- .../aggregation/any/LongAnyAggregator.java | 2 +- .../any/LongAnyAggregatorFactory.java | 4 +- .../aggregation/any/StringAnyAggregator.java | 6 +- .../any/StringAnyAggregatorFactory.java | 6 +- .../any/StringAnyBufferAggregator.java | 24 +- .../apache/druid/server/QueryLifecycle.java | 12 +- .../druid/sql/calcite/CalciteQueryTest.java | 217 ++++++++++++++++++ .../druid/sql/calcite/util/CalciteTests.java | 9 + 14 files changed, 267 insertions(+), 30 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java index caa5a26c46e6..90a2fd4fa760 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java @@ -28,7 +28,7 @@ */ public class FloatSumAggregator implements Aggregator { - public static final Comparator COMPARATOR = new Ordering() + static final Comparator COMPARATOR = new Ordering() { @Override public int compare(Object o, Object o1) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java index f9ae93c9d6ae..30b339337d16 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java @@ -29,7 +29,7 @@ */ public class LongSumAggregator implements Aggregator { - public static final Comparator COMPARATOR = new Ordering() + static final Comparator COMPARATOR = new Ordering() { @Override public int compare(Object o, Object o1) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java index c3d307a0d2b0..fcc817ae9a9d 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java @@ -29,7 +29,7 @@ */ public class DoubleAnyAggregator extends NumericAnyAggregator { - private double foundValue; + private double foundValue; public DoubleAnyAggregator(BaseDoubleColumnValueSelector valueSelector) { diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java index 1a1b5c8dc3ce..09d26812a4b6 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java @@ -44,6 +44,8 @@ public class DoubleAnyAggregatorFactory extends AggregatorFactory { + private static final Comparator VALUE_COMPARATOR = Comparator.nullsFirst(Double::compare); + private static final Aggregator NIL_AGGREGATOR = new DoubleAnyAggregator( NilColumnValueSelector.instance() ) @@ -113,8 +115,7 @@ public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) @Override public Comparator getComparator() { - //TODO check handle null - return DoubleSumAggregator.COMPARATOR; + return VALUE_COMPARATOR; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java index a13a01be133b..4e6aae55a192 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java @@ -29,7 +29,7 @@ */ public class FloatAnyAggregator extends NumericAnyAggregator { - private float foundValue; + private float foundValue; public FloatAnyAggregator(BaseFloatColumnValueSelector valueSelector) { diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java index 3d820560fdd8..fbf7ab88c79a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java @@ -51,6 +51,8 @@ public class FloatAnyAggregatorFactory extends AggregatorFactory { + private static final Comparator VALUE_COMPARATOR = Comparator.nullsFirst(Float::compare); + private static final Aggregator NIL_AGGREGATOR = new FloatAnyAggregator( NilColumnValueSelector.instance() ) @@ -118,7 +120,7 @@ public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) @Override public Comparator getComparator() { - return FloatSumAggregator.COMPARATOR; + return VALUE_COMPARATOR; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java index daeda110080e..d84855fbc0f8 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java @@ -33,7 +33,7 @@ */ public class LongAnyAggregator extends NumericAnyAggregator { - private long foundValue; + private long foundValue; public LongAnyAggregator(BaseLongColumnValueSelector valueSelector) { diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java index 0e5886c3d93b..3d8d264aca1a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java @@ -50,6 +50,8 @@ public class LongAnyAggregatorFactory extends AggregatorFactory { + private static final Comparator VALUE_COMPARATOR = Comparator.nullsFirst(Long::compare); + private static final Aggregator NIL_AGGREGATOR = new LongAnyAggregator( NilColumnValueSelector.instance() ) @@ -117,7 +119,7 @@ public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) @Override public Comparator getComparator() { - return LongSumAggregator.COMPARATOR; + return VALUE_COMPARATOR; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java index 6a60ab83ecd1..352b65e5646a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregator.java @@ -28,7 +28,7 @@ public class StringAnyAggregator implements Aggregator { private final BaseObjectColumnValueSelector valueSelector; private final int maxStringBytes; - + private boolean isFound; private String foundValue; public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxStringBytes) @@ -36,17 +36,19 @@ public StringAnyAggregator(BaseObjectColumnValueSelector valueSelector, int maxS this.valueSelector = valueSelector; this.maxStringBytes = maxStringBytes; this.foundValue = null; + this.isFound = false; } @Override public void aggregate() { - if (foundValue == null) { + if (!isFound) { final Object object = valueSelector.getObject(); foundValue = DimensionHandlerUtils.convertObjectToString(object); if (foundValue != null && foundValue.length() > maxStringBytes) { foundValue = foundValue.substring(0, maxStringBytes); } + isFound = true; } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java index 1c7022d4f25f..9e28b084bb61 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java @@ -85,11 +85,7 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - if (lhs != null) { - return lhs; - } else { - return rhs; - } + return lhs; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java index abe5ebe568ee..716f38011410 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java @@ -19,6 +19,7 @@ package org.apache.druid.query.aggregation.any; +import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.aggregation.BufferAggregator; import org.apache.druid.segment.BaseObjectColumnValueSelector; @@ -28,7 +29,13 @@ public class StringAnyBufferAggregator implements BufferAggregator { + private static final byte BYTE_FLAG_IS_NOT_SET = 0; + private static final byte BYTE_FLAG_IS_SET = 1; private static final int NULL_STRING_LENGTH = -1; + private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0; + private static final int STRING_LENGTH_OFFSET_POSITION = IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES; + private static final int FOUND_VALUE_OFFSET_POSITION = STRING_LENGTH_OFFSET_POSITION + Integer.BYTES; + private final BaseObjectColumnValueSelector valueSelector; private final int maxStringBytes; @@ -41,23 +48,26 @@ public StringAnyBufferAggregator(BaseObjectColumnValueSelector valueSelector, in @Override public void init(ByteBuffer buf, int position) { - buf.putInt(position, NULL_STRING_LENGTH); + buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); + buf.putInt(position + STRING_LENGTH_OFFSET_POSITION, NULL_STRING_LENGTH); } @Override public void aggregate(ByteBuffer buf, int position) { - int stringSizeBytes = buf.getInt(position); - if (stringSizeBytes < 0) { + if (buf.get(position + IS_FOUND_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_NOT_SET) { final Object object = valueSelector.getObject(); String foundValue = DimensionHandlerUtils.convertObjectToString(object); if (foundValue != null) { ByteBuffer mutationBuffer = buf.duplicate(); - mutationBuffer.position(position + Integer.BYTES); - mutationBuffer.limit(position + Integer.BYTES + maxStringBytes); + mutationBuffer.position(position + FOUND_VALUE_OFFSET_POSITION); + mutationBuffer.limit(position + FOUND_VALUE_OFFSET_POSITION + maxStringBytes); final int len = StringUtils.toUtf8WithLimit(foundValue, mutationBuffer); - mutationBuffer.putInt(position, len); + mutationBuffer.putInt(position + STRING_LENGTH_OFFSET_POSITION, len); + } else { + buf.putInt(position + STRING_LENGTH_OFFSET_POSITION, NULL_STRING_LENGTH); } + buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); } } @@ -65,7 +75,7 @@ public void aggregate(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { ByteBuffer copyBuffer = buf.duplicate(); - copyBuffer.position(position); + copyBuffer.position(position + STRING_LENGTH_OFFSET_POSITION); int stringSizeBytes = copyBuffer.getInt(); if (stringSizeBytes >= 0) { byte[] valueBytes = new byte[stringSizeBytes]; diff --git a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java index 6e876c4d0922..7b60188c9e44 100644 --- a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java +++ b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java @@ -318,14 +318,12 @@ public void emitLogsAndMetrics( if (e != null) { statsMap.put("exception", e.toString()); - - if (e instanceof QueryInterruptedException) { - // Mimic behavior from QueryResource, where this code was originally taken from. - log.noStackTrace().warn(e, "Exception while processing queryId [%s]", baseQuery.getId()); - statsMap.put("interrupted", true); - statsMap.put("reason", e.toString()); - } + // Mimic behavior from QueryResource, where this code was originally taken from. + log.noStackTrace().warn(e, "Exception while processing queryId [%s]", baseQuery.getId()); + statsMap.put("interrupted", e instanceof QueryInterruptedException); + statsMap.put("reason", e.toString()); } + requestLogger.logNativeQuery( RequestLogLine.forNative( baseQuery, diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 01bdc91d7db8..31b2c49adefc 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -1785,6 +1785,82 @@ public void testFirstLatestAggregatorsSkipNulls() throws Exception ); } + @Test + public void testAnyAggregatorsDoesNotSkipNulls() throws Exception + { + // Cannot vectorize ANY aggregator. + skipVectorize(); + + testQuery( + "SELECT ANY_VALUE(dim1, 32), ANY_VALUE(l2), ANY_VALUE(d2), ANY_VALUE(f2) FROM druid.numfoo", + ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .aggregators( + aggregators( + new StringAnyAggregatorFactory("a0", "dim1", 32), + new LongAnyAggregatorFactory("a1", "l2"), + new DoubleAnyAggregatorFactory("a2", "d2"), + new FloatAnyAggregatorFactory("a3", "f2") + ) + ) + .context(TIMESERIES_CONTEXT_DEFAULT) + .build() + ), + // first row has null for l2, d2, f2 and dim1 as empty string (which is null in default mode) + ImmutableList.of( + useDefault ? new Object[]{"", 0L, 0.0, 0f} : new Object[]{"", null, null, null} + ) + ); + } + + @Test + public void testAnyAggregatorsSkipNullsWithFilter() throws Exception + { + // Cannot vectorize ANY aggregator. + skipVectorize(); + + final DimFilter filter; + if (useDefault) { + filter = not(selector("dim1", null, null)); + } else { + filter = and( + not(selector("dim1", null, null)), + not(selector("l2", null, null)), + not(selector("d2", null, null)), + not(selector("f2", null, null)) + ); + } + testQuery( + "SELECT ANY_VALUE(dim1, 32), ANY_VALUE(l2), ANY_VALUE(d2), ANY_VALUE(f2) " + + "FROM druid.numfoo " + + "WHERE dim1 IS NOT NULL AND l2 IS NOT NULL AND d2 IS NOT NULL AND f2 is NOT NULL", + ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .filters(filter) + .aggregators( + aggregators( + new StringAnyAggregatorFactory("a0", "dim1", 32), + new LongAnyAggregatorFactory("a1", "l2"), + new DoubleAnyAggregatorFactory("a2", "d2"), + new FloatAnyAggregatorFactory("a3", "f2") + ) + ) + .context(TIMESERIES_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + // first row of dim1 is empty string, which is null in default mode + new Object[]{"10.1", 325323L, 1.7, 0.1f} + ) + ); + } + @Test public void testOrderByEarliestFloat() throws Exception { @@ -2067,6 +2143,147 @@ public void testOrderByLatestLong() throws Exception expected ); } +// +// @Test +// public void testOrderByAnyFloat() throws Exception +// { +// // Cannot vectorize ANY aggregator. +// skipVectorize(); +// List expected; +// if (NullHandling.replaceWithDefault()) { +// expected = ImmutableList.of( +// new Object[]{"1", 0.0f}, +// new Object[]{"2", 0.0f}, +// new Object[]{"abc", 0.0f}, +// new Object[]{"def", 0.0f}, +// new Object[]{"10.1", 0.1f}, +// new Object[]{"", 1.0f} +// ); +// } else { +// expected = ImmutableList.of( +// new Object[]{"1", null}, +// new Object[]{"abc", null}, +// new Object[]{"def", null}, +// new Object[]{"2", 0.0f}, +// new Object[]{"10.1", 0.1f}, +// new Object[]{"", 1.0f} +// ); +// } +// testQuery( +// "SELECT dim1, ANY_VALUE(f1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", +// ImmutableList.of( +// new TopNQueryBuilder() +// .dataSource(CalciteTests.DATASOURCE3) +// .intervals(querySegmentSpec(Filtration.eternity())) +// .granularity(Granularities.ALL) +// .dimension(new DefaultDimensionSpec("dim1", "_d0")) +// .aggregators( +// aggregators( +// new FloatAnyAggregatorFactory("a0", "f1") +// ) +// ) +// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) +// .threshold(10) +// .context(QUERY_CONTEXT_DEFAULT) +// .build() +// ), +// expected +// ); +// } +// +// @Test +// public void testOrderByAnyDouble() throws Exception +// { +// // Cannot vectorize ANY aggregator. +// skipVectorize(); +// List expected; +// if (NullHandling.replaceWithDefault()) { +// expected = ImmutableList.of( +// new Object[]{"1", 0.0}, +// new Object[]{"2", 0.0}, +// new Object[]{"abc", 0.0}, +// new Object[]{"def", 0.0}, +// new Object[]{"", 1.0}, +// new Object[]{"10.1", 1.7} +// ); +// } else { +// expected = ImmutableList.of( +// new Object[]{"1", null}, +// new Object[]{"abc", null}, +// new Object[]{"def", null}, +// new Object[]{"2", 0.0}, +// new Object[]{"", 1.0}, +// new Object[]{"10.1", 1.7} +// ); +// } +// testQuery( +// "SELECT dim1, ANY_VALUE(d1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", +// ImmutableList.of( +// new TopNQueryBuilder() +// .dataSource(CalciteTests.DATASOURCE3) +// .intervals(querySegmentSpec(Filtration.eternity())) +// .granularity(Granularities.ALL) +// .dimension(new DefaultDimensionSpec("dim1", "_d0")) +// .aggregators( +// aggregators( +// new DoubleAnyAggregatorFactory("a0", "d1") +// ) +// ) +// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) +// .threshold(10) +// .context(QUERY_CONTEXT_DEFAULT) +// .build() +// ), +// expected +// ); +// } +// +// @Test +// public void testOrderByAnyLong() throws Exception +// { +// // Cannot vectorize ANY aggregator. +// skipVectorize(); +// List expected; +// if (NullHandling.replaceWithDefault()) { +// expected = ImmutableList.of( +// new Object[]{"1", 0L}, +// new Object[]{"2", 0L}, +// new Object[]{"abc", 0L}, +// new Object[]{"def", 0L}, +// new Object[]{"", 7L}, +// new Object[]{"10.1", 325323L} +// ); +// } else { +// expected = ImmutableList.of( +// new Object[]{"1", null}, +// new Object[]{"abc", null}, +// new Object[]{"def", null}, +// new Object[]{"2", 0L}, +// new Object[]{"", 7L}, +// new Object[]{"10.1", 325323L} +// ); +// } +// testQuery( +// "SELECT dim1, ANY_VALUE(l1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", +// ImmutableList.of( +// new TopNQueryBuilder() +// .dataSource(CalciteTests.DATASOURCE3) +// .intervals(querySegmentSpec(Filtration.eternity())) +// .granularity(Granularities.ALL) +// .dimension(new DefaultDimensionSpec("dim1", "_d0")) +// .aggregators( +// aggregators( +// new LongAnyAggregatorFactory("a0", "l1") +// ) +// ) +// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) +// .threshold(10) +// .context(QUERY_CONTEXT_DEFAULT) +// .build() +// ), +// expected +// ); +// } @Test public void testGroupByLong() throws Exception diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java index 51a505de686e..30942eedb275 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java @@ -269,8 +269,11 @@ public AuthenticationResult createEscalatedAuthenticationResult() ImmutableList.builder() .addAll(DimensionsSpec.getDefaultSchemas(ImmutableList.of("dim1", "dim2", "dim3"))) .add(new DoubleDimensionSchema("d1")) + .add(new DoubleDimensionSchema("d2")) .add(new FloatDimensionSchema("f1")) + .add(new FloatDimensionSchema("f2")) .add(new LongDimensionSchema("l1")) + .add(new LongDimensionSchema("l2")) .build(), null, null @@ -414,8 +417,11 @@ public AuthenticationResult createEscalatedAuthenticationResult() .put("m1", "2.0") .put("m2", "2.0") .put("d1", 1.7) + .put("d2", 1.7) .put("f1", 0.1f) + .put("f2", 0.1f) .put("l1", 325323L) + .put("l2", 325323L) .put("dim1", "10.1") .put("dim2", ImmutableList.of()) .put("dim3", ImmutableList.of("b", "c")) @@ -428,8 +434,11 @@ public AuthenticationResult createEscalatedAuthenticationResult() .put("m1", "3.0") .put("m2", "3.0") .put("d1", 0.0) + .put("d2", 0.0) .put("f1", 0.0) + .put("f2", 0.0) .put("l1", 0) + .put("l2", 0) .put("dim1", "2") .put("dim2", ImmutableList.of("")) .put("dim3", ImmutableList.of("d")) From 68ac8de271d290f798b073e699a9d08aea1dee1b Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 6 Feb 2020 16:35:30 -0800 Subject: [PATCH 3/9] add more tests --- .../any/StringAnyAggregatorFactory.java | 2 +- .../any/DoubleAnyAggregationTest.java | 185 +++++++++++ .../any/FloatAnyAggregationTest.java | 185 +++++++++++ .../any/LongAnyAggregationTest.java | 186 +++++++++++ .../any/StringAnyAggregationTest.java | 160 ++++++++++ .../any/StringAnyBufferAggregatorTest.java | 171 ++++++++++ .../druid/sql/calcite/CalciteQueryTest.java | 294 +++++++++--------- 7 files changed, 1039 insertions(+), 144 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java index 9e28b084bb61..1eb9d7a9f259 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java @@ -155,7 +155,7 @@ public String getTypeName() @Override public int getMaxIntermediateSize() { - return Integer.BYTES + maxStringBytes; + return Byte.BYTES + Integer.BYTES + maxStringBytes; } @Override diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java new file mode 100644 index 000000000000..8866cebaa47c --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java @@ -0,0 +1,185 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.jackson.DefaultObjectMapper; +import org.apache.druid.query.aggregation.Aggregator; +import org.apache.druid.query.aggregation.AggregatorFactory; +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.TestDoubleColumnSelectorImpl; +import org.apache.druid.query.aggregation.TestObjectColumnSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.easymock.EasyMock; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.ByteBuffer; +import java.util.Comparator; + +public class DoubleAnyAggregationTest extends InitializedNullHandlingTest +{ + private DoubleAnyAggregatorFactory doubleAnyAggFactory; + private DoubleAnyAggregatorFactory combiningAggFactory; + private ColumnSelectorFactory colSelectorFactory; + private TestDoubleColumnSelectorImpl valueSelector; + private TestObjectColumnSelector objectSelector; + + private double[] doubles = {1.1897d, 0.001d, 86.23d, 166.228d}; + private Double[] objects = {2.1897d, 1.001d, 87.23d, 167.228d}; + + @Before + public void setup() + { + doubleAnyAggFactory = new DoubleAnyAggregatorFactory("billy", "nilly"); + combiningAggFactory = (DoubleAnyAggregatorFactory) doubleAnyAggFactory.getCombiningFactory(); + valueSelector = new TestDoubleColumnSelectorImpl(doubles); + objectSelector = new TestObjectColumnSelector<>(objects); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("billy")).andReturn(objectSelector); + EasyMock.replay(colSelectorFactory); + } + + @Test + public void testDoubleAnyAggregator() + { + Aggregator agg = doubleAnyAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Double result = (Double) agg.get(); + + Assert.assertEquals((Double) doubles[0], result); + Assert.assertEquals((long) doubles[0], agg.getLong()); + Assert.assertEquals(doubles[0], agg.getDouble(), 0.0001); + } + + @Test + public void testDoubleAnyBufferAggregator() + { + BufferAggregator agg = doubleAnyAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[doubleAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Double result = (Double) agg.get(buffer, 0); + + Assert.assertEquals(doubles[0], result, 0.0001); + Assert.assertEquals((long) doubles[0], agg.getLong(buffer, 0)); + Assert.assertEquals(doubles[0], agg.getDouble(buffer, 0), 0.0001); + } + + @Test + public void testCombine() + { + Double d1 = 3.0; + Double d2 = 4.0; + Assert.assertEquals(d1, doubleAnyAggFactory.combine(d1, d2)); + } + + @Test + public void testComparatorWithNulls() + { + Double d1 = 3.0; + Double d2 = null; + Comparator comparator = doubleAnyAggFactory.getComparator(); + Assert.assertEquals(1, comparator.compare(d1, d2)); + Assert.assertEquals(0, comparator.compare(d1, d1)); + Assert.assertEquals(0, comparator.compare(d2, d2)); + Assert.assertEquals(-1, comparator.compare(d2, d1)); + } + + @Test + public void testDoubleAnyCombiningAggregator() + { + Aggregator agg = combiningAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Double result = (Double) agg.get(); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong()); + Assert.assertEquals(objects[0], agg.getDouble(), 0.0001); + } + + @Test + public void testDoubleAnyCombiningBufferAggregator() + { + BufferAggregator agg = combiningAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[doubleAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Double result = (Double) agg.get(buffer, 0); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong(buffer, 0)); + Assert.assertEquals(objects[0], agg.getDouble(buffer, 0), 0.0001); + } + + @Test + public void testSerde() throws Exception + { + DefaultObjectMapper mapper = new DefaultObjectMapper(); + String doubleSpecJson = "{\"type\":\"doubleAny\",\"name\":\"billy\",\"fieldName\":\"nilly\"}"; + Assert.assertEquals(doubleAnyAggFactory, mapper.readValue(doubleSpecJson, AggregatorFactory.class)); + } + + private void aggregate( + Aggregator agg + ) + { + agg.aggregate(); + valueSelector.increment(); + objectSelector.increment(); + } + + private void aggregate( + BufferAggregator agg, + ByteBuffer buff, + int position + ) + { + agg.aggregate(buff, position); + valueSelector.increment(); + objectSelector.increment(); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java new file mode 100644 index 000000000000..d04713d26330 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java @@ -0,0 +1,185 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.jackson.DefaultObjectMapper; +import org.apache.druid.query.aggregation.Aggregator; +import org.apache.druid.query.aggregation.AggregatorFactory; +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.TestFloatColumnSelector; +import org.apache.druid.query.aggregation.TestObjectColumnSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.easymock.EasyMock; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.ByteBuffer; +import java.util.Comparator; + +public class FloatAnyAggregationTest extends InitializedNullHandlingTest +{ + private FloatAnyAggregatorFactory floatAnyAggFactory; + private FloatAnyAggregatorFactory combiningAggFactory; + private ColumnSelectorFactory colSelectorFactory; + private TestFloatColumnSelector valueSelector; + private TestObjectColumnSelector objectSelector; + + private float[] floats = {1.1897f, 0.001f, 86.23f, 166.228f}; + private Float[] objects = {2.1897f, 1.001f, 87.23f, 167.228f}; + + @Before + public void setup() + { + floatAnyAggFactory = new FloatAnyAggregatorFactory("billy", "nilly"); + combiningAggFactory = (FloatAnyAggregatorFactory) floatAnyAggFactory.getCombiningFactory(); + valueSelector = new TestFloatColumnSelector(floats); + objectSelector = new TestObjectColumnSelector<>(objects); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("billy")).andReturn(objectSelector); + EasyMock.replay(colSelectorFactory); + } + + @Test + public void testFloatAnyAggregator() + { + Aggregator agg = floatAnyAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Float result = (Float) agg.get(); + + Assert.assertEquals((Float) floats[0], result); + Assert.assertEquals((long) floats[0], agg.getLong()); + Assert.assertEquals(floats[0], agg.getFloat(), 0.0001); + } + + @Test + public void testFloatAnyBufferAggregator() + { + BufferAggregator agg = floatAnyAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[floatAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Float result = (Float) agg.get(buffer, 0); + + Assert.assertEquals(floats[0], result, 0.0001); + Assert.assertEquals((long) floats[0], agg.getLong(buffer, 0)); + Assert.assertEquals(floats[0], agg.getFloat(buffer, 0), 0.0001); + } + + @Test + public void testCombine() + { + Float f1 = 3.0f; + Float f2 = 4.0f; + Assert.assertEquals(f1, floatAnyAggFactory.combine(f1, f2)); + } + + @Test + public void testComparatorWithNulls() + { + Float f1 = 3.0f; + Float f2 = null; + Comparator comparator = floatAnyAggFactory.getComparator(); + Assert.assertEquals(1, comparator.compare(f1, f2)); + Assert.assertEquals(0, comparator.compare(f1, f1)); + Assert.assertEquals(0, comparator.compare(f2, f2)); + Assert.assertEquals(-1, comparator.compare(f2, f1)); + } + + @Test + public void testFloatAnyCombiningAggregator() + { + Aggregator agg = combiningAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Float result = (Float) agg.get(); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong()); + Assert.assertEquals(objects[0], agg.getFloat(), 0.0001); + } + + @Test + public void testFloatAnyCombiningBufferAggregator() + { + BufferAggregator agg = combiningAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[floatAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Float result = (Float) agg.get(buffer, 0); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong(buffer, 0)); + Assert.assertEquals(objects[0], agg.getFloat(buffer, 0), 0.0001); + } + + @Test + public void testSerde() throws Exception + { + DefaultObjectMapper mapper = new DefaultObjectMapper(); + String floatSpecJson = "{\"type\":\"floatAny\",\"name\":\"billy\",\"fieldName\":\"nilly\"}"; + Assert.assertEquals(floatAnyAggFactory, mapper.readValue(floatSpecJson, AggregatorFactory.class)); + } + + private void aggregate( + Aggregator agg + ) + { + agg.aggregate(); + valueSelector.increment(); + objectSelector.increment(); + } + + private void aggregate( + BufferAggregator agg, + ByteBuffer buff, + int position + ) + { + agg.aggregate(buff, position); + valueSelector.increment(); + objectSelector.increment(); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java new file mode 100644 index 000000000000..89cb8ec8cfa9 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java @@ -0,0 +1,186 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.jackson.DefaultObjectMapper; +import org.apache.druid.query.aggregation.Aggregator; +import org.apache.druid.query.aggregation.AggregatorFactory; +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.TestLongColumnSelector; +import org.apache.druid.query.aggregation.TestObjectColumnSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.easymock.EasyMock; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.ByteBuffer; +import java.util.Comparator; + +public class LongAnyAggregationTest extends InitializedNullHandlingTest +{ + + private LongAnyAggregatorFactory longAnyAggFactory; + private LongAnyAggregatorFactory combiningAggFactory; + private ColumnSelectorFactory colSelectorFactory; + private TestLongColumnSelector valueSelector; + private TestObjectColumnSelector objectSelector; + + private long[] longs = {185, -216, -128751132, Long.MIN_VALUE}; + private Long[] objects = {1123126751L, 1784247991L, 1854329816L, 1000000000L}; + + @Before + public void setup() + { + longAnyAggFactory = new LongAnyAggregatorFactory("billy", "nilly"); + combiningAggFactory = (LongAnyAggregatorFactory) longAnyAggFactory.getCombiningFactory(); + valueSelector = new TestLongColumnSelector(longs); + objectSelector = new TestObjectColumnSelector<>(objects); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("billy")).andReturn(objectSelector); + EasyMock.replay(colSelectorFactory); + } + + @Test + public void testLongAnyAggregator() + { + Aggregator agg = longAnyAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Long result = (Long) agg.get(); + + Assert.assertEquals((Long) longs[0], result); + Assert.assertEquals((long) longs[0], agg.getLong()); + Assert.assertEquals(longs[0], agg.getLong(), 0.0001); + } + + @Test + public void testLongAnyBufferAggregator() + { + BufferAggregator agg = longAnyAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[longAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Long result = (Long) agg.get(buffer, 0); + + Assert.assertEquals(longs[0], result, 0.0001); + Assert.assertEquals((long) longs[0], agg.getLong(buffer, 0)); + Assert.assertEquals(longs[0], agg.getLong(buffer, 0), 0.0001); + } + + @Test + public void testCombine() + { + Long l1 = 3L; + Long l2 = 4L; + Assert.assertEquals(l1, longAnyAggFactory.combine(l1, l2)); + } + + @Test + public void testComparatorWithNulls() + { + Long l1 = 3L; + Long l2 = null; + Comparator comparator = longAnyAggFactory.getComparator(); + Assert.assertEquals(1, comparator.compare(l1, l2)); + Assert.assertEquals(0, comparator.compare(l1, l1)); + Assert.assertEquals(0, comparator.compare(l2, l2)); + Assert.assertEquals(-1, comparator.compare(l2, l1)); + } + + @Test + public void testLongAnyCombiningAggregator() + { + Aggregator agg = combiningAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + Long result = (Long) agg.get(); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong()); + Assert.assertEquals(objects[0], agg.getLong(), 0.0001); + } + + @Test + public void testLongAnyCombiningBufferAggregator() + { + BufferAggregator agg = combiningAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[longAnyAggFactory.getMaxIntermediateSizeWithNulls()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + Long result = (Long) agg.get(buffer, 0); + + Assert.assertEquals(objects[0], result, 0.0001); + Assert.assertEquals(objects[0].longValue(), agg.getLong(buffer, 0)); + Assert.assertEquals(objects[0], agg.getLong(buffer, 0), 0.0001); + } + + @Test + public void testSerde() throws Exception + { + DefaultObjectMapper mapper = new DefaultObjectMapper(); + String longSpecJson = "{\"type\":\"longAny\",\"name\":\"billy\",\"fieldName\":\"nilly\"}"; + Assert.assertEquals(longAnyAggFactory, mapper.readValue(longSpecJson, AggregatorFactory.class)); + } + + private void aggregate( + Aggregator agg + ) + { + agg.aggregate(); + valueSelector.increment(); + objectSelector.increment(); + } + + private void aggregate( + BufferAggregator agg, + ByteBuffer buff, + int position + ) + { + agg.aggregate(buff, position); + valueSelector.increment(); + objectSelector.increment(); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java new file mode 100644 index 000000000000..8084cfcb156a --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java @@ -0,0 +1,160 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.query.aggregation.Aggregator; +import org.apache.druid.query.aggregation.AggregatorFactory; +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.TestObjectColumnSelector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.segment.column.ColumnCapabilitiesImpl; +import org.apache.druid.segment.column.ValueType; +import org.easymock.EasyMock; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.ByteBuffer; + +public class StringAnyAggregationTest +{ + private final Integer MAX_STRING_SIZE = 1024; + private AggregatorFactory stringAnyAggFactory; + private AggregatorFactory combiningAggFactory; + private ColumnSelectorFactory colSelectorFactory; + private TestObjectColumnSelector valueSelector; + private TestObjectColumnSelector objectSelector; + + private String[] strings = {"1111", "2222", "3333", null, "4444"}; + + @Before + public void setup() + { + stringAnyAggFactory = new StringAnyAggregatorFactory("billy", "nilly", MAX_STRING_SIZE); + combiningAggFactory = stringAnyAggFactory.getCombiningFactory(); + valueSelector = new TestObjectColumnSelector<>(strings); + objectSelector = new TestObjectColumnSelector<>(strings); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("billy")).andReturn(objectSelector); + EasyMock.expect(colSelectorFactory.getColumnCapabilities("nilly")) + .andReturn(new ColumnCapabilitiesImpl().setType(ValueType.STRING)); + EasyMock.expect(colSelectorFactory.getColumnCapabilities("billy")).andReturn(null); + EasyMock.replay(colSelectorFactory); + } + + @Test + public void testStringAnyAggregator() + { + Aggregator agg = stringAnyAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + String result = (String) agg.get(); + + Assert.assertEquals(strings[0], result); + } + + @Test + public void testStringAnyBufferAggregator() + { + BufferAggregator agg = stringAnyAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[stringAnyAggFactory.getMaxIntermediateSize()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + String result = (String) agg.get(buffer, 0); + + Assert.assertEquals(strings[0], result); + } + + @Test + public void testCombine() + { + String s1 = "aaaaaa"; + String s2 = "aaaaaa"; + Assert.assertEquals(s1, stringAnyAggFactory.combine(s1, s2)); + } + + @Test + public void testStringAnyCombiningAggregator() + { + Aggregator agg = combiningAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + String result = (String) agg.get(); + + Assert.assertEquals(strings[0], result); + Assert.assertEquals(strings[0], result); + } + + @Test + public void testStringAnyCombiningBufferAggregator() + { + BufferAggregator agg = combiningAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[stringAnyAggFactory.getMaxIntermediateSize()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + String result = (String) agg.get(buffer, 0); + + Assert.assertEquals(strings[0], result); + Assert.assertEquals(strings[0], result); + } + + private void aggregate( + Aggregator agg + ) + { + agg.aggregate(); + valueSelector.increment(); + objectSelector.increment(); + } + + private void aggregate( + BufferAggregator agg, + ByteBuffer buff, + int position + ) + { + agg.aggregate(buff, position); + valueSelector.increment(); + objectSelector.increment(); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java new file mode 100644 index 000000000000..c8e886b58ceb --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java @@ -0,0 +1,171 @@ +/* + * 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.aggregation.any; + +import org.apache.druid.query.aggregation.BufferAggregator; +import org.apache.druid.query.aggregation.TestObjectColumnSelector; +import org.junit.Assert; +import org.junit.Test; + +import java.nio.ByteBuffer; + +public class StringAnyBufferAggregatorTest +{ + private void aggregateBuffer( + TestObjectColumnSelector valueSelector, + BufferAggregator agg, + ByteBuffer buf, + int position + ) + { + agg.aggregate(buf, position); + valueSelector.increment(); + } + + @Test + public void testBufferAggregate() + { + + final String[] strings = {"AAAA", "BBBB", "CCCC", "DDDD", "EEEE"}; + Integer maxStringBytes = 1024; + + TestObjectColumnSelector objectColumnSelector = new TestObjectColumnSelector<>(strings); + + StringAnyAggregatorFactory factory = new StringAnyAggregatorFactory( + "billy", "billy", maxStringBytes + ); + + StringAnyBufferAggregator agg = new StringAnyBufferAggregator( + objectColumnSelector, + maxStringBytes + ); + + ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize()); + int position = 0; + + agg.init(buf, position); + //noinspection ForLoopReplaceableByForEach + for (int i = 0; i < strings.length; i++) { + aggregateBuffer(objectColumnSelector, agg, buf, position); + } + + String result = ((String) agg.get(buf, position)); + + Assert.assertEquals("expected any string value", strings[0], result); + } + + @Test + public void testBufferAggregateWithFoldCheck() + { + final String[] strings = {"AAAA", "BBBB", "CCCC", "DDDD", "EEEE"}; + Integer maxStringBytes = 1024; + + TestObjectColumnSelector objectColumnSelector = new TestObjectColumnSelector<>(strings); + + StringAnyAggregatorFactory factory = new StringAnyAggregatorFactory( + "billy", "billy", maxStringBytes + ); + + StringAnyBufferAggregator agg = new StringAnyBufferAggregator( + objectColumnSelector, + maxStringBytes + ); + + ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize()); + int position = 0; + + agg.init(buf, position); + //noinspection ForLoopReplaceableByForEach + for (int i = 0; i < strings.length; i++) { + aggregateBuffer(objectColumnSelector, agg, buf, position); + } + + String result = ((String) agg.get(buf, position)); + + + Assert.assertEquals("expected any string value", strings[0], result); + } + + @Test + public void testNullBufferAggregate() + { + + final String[] strings = {"CCCC", "AAAA", "BBBB", null, "EEEE"}; + Integer maxStringBytes = 1024; + + TestObjectColumnSelector objectColumnSelector = new TestObjectColumnSelector<>(strings); + + StringAnyAggregatorFactory factory = new StringAnyAggregatorFactory( + "billy", "billy", maxStringBytes + ); + + StringAnyBufferAggregator agg = new StringAnyBufferAggregator( + objectColumnSelector, + maxStringBytes + ); + + ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize()); + int position = 0; + + agg.init(buf, position); + //noinspection ForLoopReplaceableByForEach + for (int i = 0; i < strings.length; i++) { + aggregateBuffer(objectColumnSelector, agg, buf, position); + } + + String result = ((String) agg.get(buf, position)); + + + Assert.assertEquals("expected any string value", strings[0], result); + + } + + @Test + public void testNonStringValue() + { + + final Double[] doubles = {1.00, 2.00}; + Integer maxStringBytes = 1024; + + TestObjectColumnSelector objectColumnSelector = new TestObjectColumnSelector<>(doubles); + + StringAnyAggregatorFactory factory = new StringAnyAggregatorFactory( + "billy", "billy", maxStringBytes + ); + + StringAnyBufferAggregator agg = new StringAnyBufferAggregator( + objectColumnSelector, + maxStringBytes + ); + + ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize()); + int position = 0; + + agg.init(buf, position); + //noinspection ForLoopReplaceableByForEach + for (int i = 0; i < doubles.length; i++) { + aggregateBuffer(objectColumnSelector, agg, buf, position); + } + + String result = ((String) agg.get(buf, position)); + + Assert.assertEquals("1.0", result); + } +} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 31b2c49adefc..ed39d29ecf8c 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -21,6 +21,7 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.tools.ValidationException; import org.apache.druid.common.config.NullHandling; @@ -1339,7 +1340,7 @@ public void testAnyAggregator() throws Exception .context(TIMESERIES_CONTEXT_DEFAULT) .build() ), - NullHandling.sqlCompatible() ? ImmutableList.of(new Object[]{1L, 1.0f, 1.0, "", 2L, 2.0f, "1"}) : ImmutableList.of(new Object[]{1L, 1.0f, 1.0, "10.1", 2L, 2.0f, "1"}) + NullHandling.sqlCompatible() ? ImmutableList.of(new Object[]{1L, 1.0f, 1.0, "", 2L, 2.0f, "1"}) : ImmutableList.of(new Object[]{1L, 1.0f, 1.0, "", 2L, 2.0f, "1"}) ); } @@ -1677,7 +1678,7 @@ public void testStringAnyInSubquery() throws Exception .build() ), ImmutableList.of( - new Object[]{NullHandling.sqlCompatible() ? 12.1 : 11.1} + new Object[]{NullHandling.sqlCompatible() ? 12.1 : 10.1} ) ); } @@ -2143,147 +2144,154 @@ public void testOrderByLatestLong() throws Exception expected ); } -// -// @Test -// public void testOrderByAnyFloat() throws Exception -// { -// // Cannot vectorize ANY aggregator. -// skipVectorize(); -// List expected; -// if (NullHandling.replaceWithDefault()) { -// expected = ImmutableList.of( -// new Object[]{"1", 0.0f}, -// new Object[]{"2", 0.0f}, -// new Object[]{"abc", 0.0f}, -// new Object[]{"def", 0.0f}, -// new Object[]{"10.1", 0.1f}, -// new Object[]{"", 1.0f} -// ); -// } else { -// expected = ImmutableList.of( -// new Object[]{"1", null}, -// new Object[]{"abc", null}, -// new Object[]{"def", null}, -// new Object[]{"2", 0.0f}, -// new Object[]{"10.1", 0.1f}, -// new Object[]{"", 1.0f} -// ); -// } -// testQuery( -// "SELECT dim1, ANY_VALUE(f1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", -// ImmutableList.of( -// new TopNQueryBuilder() -// .dataSource(CalciteTests.DATASOURCE3) -// .intervals(querySegmentSpec(Filtration.eternity())) -// .granularity(Granularities.ALL) -// .dimension(new DefaultDimensionSpec("dim1", "_d0")) -// .aggregators( -// aggregators( -// new FloatAnyAggregatorFactory("a0", "f1") -// ) -// ) -// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) -// .threshold(10) -// .context(QUERY_CONTEXT_DEFAULT) -// .build() -// ), -// expected -// ); -// } -// -// @Test -// public void testOrderByAnyDouble() throws Exception -// { -// // Cannot vectorize ANY aggregator. -// skipVectorize(); -// List expected; -// if (NullHandling.replaceWithDefault()) { -// expected = ImmutableList.of( -// new Object[]{"1", 0.0}, -// new Object[]{"2", 0.0}, -// new Object[]{"abc", 0.0}, -// new Object[]{"def", 0.0}, -// new Object[]{"", 1.0}, -// new Object[]{"10.1", 1.7} -// ); -// } else { -// expected = ImmutableList.of( -// new Object[]{"1", null}, -// new Object[]{"abc", null}, -// new Object[]{"def", null}, -// new Object[]{"2", 0.0}, -// new Object[]{"", 1.0}, -// new Object[]{"10.1", 1.7} -// ); -// } -// testQuery( -// "SELECT dim1, ANY_VALUE(d1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", -// ImmutableList.of( -// new TopNQueryBuilder() -// .dataSource(CalciteTests.DATASOURCE3) -// .intervals(querySegmentSpec(Filtration.eternity())) -// .granularity(Granularities.ALL) -// .dimension(new DefaultDimensionSpec("dim1", "_d0")) -// .aggregators( -// aggregators( -// new DoubleAnyAggregatorFactory("a0", "d1") -// ) -// ) -// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) -// .threshold(10) -// .context(QUERY_CONTEXT_DEFAULT) -// .build() -// ), -// expected -// ); -// } -// -// @Test -// public void testOrderByAnyLong() throws Exception -// { -// // Cannot vectorize ANY aggregator. -// skipVectorize(); -// List expected; -// if (NullHandling.replaceWithDefault()) { -// expected = ImmutableList.of( -// new Object[]{"1", 0L}, -// new Object[]{"2", 0L}, -// new Object[]{"abc", 0L}, -// new Object[]{"def", 0L}, -// new Object[]{"", 7L}, -// new Object[]{"10.1", 325323L} -// ); -// } else { -// expected = ImmutableList.of( -// new Object[]{"1", null}, -// new Object[]{"abc", null}, -// new Object[]{"def", null}, -// new Object[]{"2", 0L}, -// new Object[]{"", 7L}, -// new Object[]{"10.1", 325323L} -// ); -// } -// testQuery( -// "SELECT dim1, ANY_VALUE(l1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", -// ImmutableList.of( -// new TopNQueryBuilder() -// .dataSource(CalciteTests.DATASOURCE3) -// .intervals(querySegmentSpec(Filtration.eternity())) -// .granularity(Granularities.ALL) -// .dimension(new DefaultDimensionSpec("dim1", "_d0")) -// .aggregators( -// aggregators( -// new LongAnyAggregatorFactory("a0", "l1") -// ) -// ) -// .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) -// .threshold(10) -// .context(QUERY_CONTEXT_DEFAULT) -// .build() -// ), -// expected -// ); -// } + + @Test + public void testOrderByAnyFloat() throws Exception + { + // Cannot vectorize ANY aggregator. + skipVectorize(); + List expected; + if (NullHandling.replaceWithDefault()) { + expected = ImmutableList.of( + new Object[]{"1", 0.0f}, + new Object[]{"2", 0.0f}, + new Object[]{"abc", 0.0f}, + new Object[]{"def", 0.0f}, + new Object[]{"10.1", 0.1f}, + new Object[]{"", 1.0f} + ); + } else { + expected = ImmutableList.of( + new Object[]{"2", 0.0f}, + new Object[]{"10.1", 0.1f}, + new Object[]{"", 1.0f}, + // Nulls are last because of the null first wrapped Comparator in InvertedTopNMetricSpec which is then + // reversed by TopNNumericResultBuilder.build() + new Object[]{"1", null}, + new Object[]{"abc", null}, + new Object[]{"def", null} + ); + } + + testQuery( + "SELECT dim1, ANY_VALUE(f1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", + ImmutableList.of( + new TopNQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .dimension(new DefaultDimensionSpec("dim1", "_d0")) + .aggregators( + aggregators( + new FloatAnyAggregatorFactory("a0", "f1") + ) + ) + .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) + .threshold(10) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + expected + ); + } + + @Test + public void testOrderByAnyDouble() throws Exception + { + // Cannot vectorize ANY aggregator. + skipVectorize(); + List expected; + if (NullHandling.replaceWithDefault()) { + expected = ImmutableList.of( + new Object[]{"1", 0.0}, + new Object[]{"2", 0.0}, + new Object[]{"abc", 0.0}, + new Object[]{"def", 0.0}, + new Object[]{"", 1.0}, + new Object[]{"10.1", 1.7} + ); + } else { + expected = ImmutableList.of( + new Object[]{"2", 0.0}, + new Object[]{"", 1.0}, + new Object[]{"10.1", 1.7}, + // Nulls are last because of the null first wrapped Comparator in InvertedTopNMetricSpec which is then + // reversed by TopNNumericResultBuilder.build() + new Object[]{"1", null}, + new Object[]{"abc", null}, + new Object[]{"def", null} + ); + } + testQuery( + "SELECT dim1, ANY_VALUE(d1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", + ImmutableList.of( + new TopNQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .dimension(new DefaultDimensionSpec("dim1", "_d0")) + .aggregators( + aggregators( + new DoubleAnyAggregatorFactory("a0", "d1") + ) + ) + .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) + .threshold(10) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + expected + ); + } + + @Test + public void testOrderByAnyLong() throws Exception + { + // Cannot vectorize ANY aggregator. + skipVectorize(); + List expected; + if (NullHandling.replaceWithDefault()) { + expected = ImmutableList.of( + new Object[]{"1", 0L}, + new Object[]{"2", 0L}, + new Object[]{"abc", 0L}, + new Object[]{"def", 0L}, + new Object[]{"", 7L}, + new Object[]{"10.1", 325323L} + ); + } else { + expected = ImmutableList.of( + new Object[]{"2", 0L}, + new Object[]{"", 7L}, + new Object[]{"10.1", 325323L}, + // Nulls are last because of the null first wrapped Comparator in InvertedTopNMetricSpec which is then + // reversed by TopNNumericResultBuilder.build() + new Object[]{"1", null}, + new Object[]{"abc", null}, + new Object[]{"def", null} + ); + } + testQuery( + "SELECT dim1, ANY_VALUE(l1) FROM druid.numfoo GROUP BY 1 ORDER BY 2 LIMIT 10", + ImmutableList.of( + new TopNQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.ALL) + .dimension(new DefaultDimensionSpec("dim1", "_d0")) + .aggregators( + aggregators( + new LongAnyAggregatorFactory("a0", "l1") + ) + ) + .metric(new InvertedTopNMetricSpec(new NumericTopNMetricSpec("a0"))) + .threshold(10) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + expected + ); + } @Test public void testGroupByLong() throws Exception From e68e9ee9b3ee94ba3ff746c0c2c8d994b49110c4 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 6 Feb 2020 16:51:19 -0800 Subject: [PATCH 4/9] Update documentation --- docs/querying/aggregations.md | 2 +- docs/querying/sql.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/querying/aggregations.md b/docs/querying/aggregations.md index 4209d9accb2c..024a43b546fc 100644 --- a/docs/querying/aggregations.md +++ b/docs/querying/aggregations.md @@ -238,7 +238,7 @@ Note that queries with first/last aggregators on a segment created with rollup e (Double/Float/Long/String) ANY aggregator cannot be used in ingestion spec, and should only be specified as part of queries. -If `druid.generic.useDefaultValueForNull=true` aggregation can returns the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then aggregation will returns any non-null value. +If `druid.generic.useDefaultValueForNull=true` aggregation can returns the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then aggregation will returns any value including null. #### `doubleAny` aggregator diff --git a/docs/querying/sql.md b/docs/querying/sql.md index 0498f81c268c..29bad8c477a5 100644 --- a/docs/querying/sql.md +++ b/docs/querying/sql.md @@ -205,7 +205,7 @@ Only the COUNT aggregation can accept DISTINCT. |`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| |`LATEST(expr)`|Returns the latest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "latest" is the value last encountered with the maximum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the last value encountered.| |`LATEST(expr, maxBytesPerString)`|Like `LATEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| -|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any non-null value of `expr`| +|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any value of `expr` including null| |`ANY_VALUE(expr, maxBytesPerString)`|Like `ANY_VALUE(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| From e224eedf9ab1b385447d677b38d39c5f58eafe73 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 6 Feb 2020 17:38:36 -0800 Subject: [PATCH 5/9] add more tests --- .../any/StringAnyAggregationTest.java | 48 +++++++++++++++++++ .../any/StringAnyBufferAggregatorTest.java | 38 +++++++++++++-- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java index 8084cfcb156a..2d2d19e0d8f1 100644 --- a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyAggregationTest.java @@ -43,6 +43,8 @@ public class StringAnyAggregationTest private TestObjectColumnSelector objectSelector; private String[] strings = {"1111", "2222", "3333", null, "4444"}; + private String[] stringsWithNullFirst = {null, "1111", "2222", "3333", null, "4444"}; + @Before public void setup() @@ -75,6 +77,27 @@ public void testStringAnyAggregator() Assert.assertEquals(strings[0], result); } + @Test + public void testStringAnyAggregatorWithNullFirst() + { + valueSelector = new TestObjectColumnSelector<>(stringsWithNullFirst); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.getColumnCapabilities("nilly")) + .andReturn(new ColumnCapabilitiesImpl().setType(ValueType.STRING)); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.replay(colSelectorFactory); + + Aggregator agg = stringAnyAggFactory.factorize(colSelectorFactory); + + aggregate(agg); + aggregate(agg); + aggregate(agg); + aggregate(agg); + + String result = (String) agg.get(); + Assert.assertNull(result); + } + @Test public void testStringAnyBufferAggregator() { @@ -94,6 +117,31 @@ public void testStringAnyBufferAggregator() Assert.assertEquals(strings[0], result); } + @Test + public void testStringAnyBufferAggregatorWithNullFirst() + { + valueSelector = new TestObjectColumnSelector<>(stringsWithNullFirst); + colSelectorFactory = EasyMock.createMock(ColumnSelectorFactory.class); + EasyMock.expect(colSelectorFactory.getColumnCapabilities("nilly")) + .andReturn(new ColumnCapabilitiesImpl().setType(ValueType.STRING)); + EasyMock.expect(colSelectorFactory.makeColumnValueSelector("nilly")).andReturn(valueSelector); + EasyMock.replay(colSelectorFactory); + + BufferAggregator agg = stringAnyAggFactory.factorizeBuffered( + colSelectorFactory); + + ByteBuffer buffer = ByteBuffer.wrap(new byte[stringAnyAggFactory.getMaxIntermediateSize()]); + agg.init(buffer, 0); + + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + aggregate(agg, buffer, 0); + + String result = (String) agg.get(buffer, 0); + Assert.assertNull(result); + } + @Test public void testCombine() { diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java index c8e886b58ceb..658db6f7eec3 100644 --- a/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java +++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregatorTest.java @@ -68,7 +68,7 @@ public void testBufferAggregate() String result = ((String) agg.get(buf, position)); - Assert.assertEquals("expected any string value", strings[0], result); + Assert.assertEquals(strings[0], result); } @Test @@ -100,11 +100,11 @@ public void testBufferAggregateWithFoldCheck() String result = ((String) agg.get(buf, position)); - Assert.assertEquals("expected any string value", strings[0], result); + Assert.assertEquals(strings[0], result); } @Test - public void testNullBufferAggregate() + public void testContainsNullBufferAggregate() { final String[] strings = {"CCCC", "AAAA", "BBBB", null, "EEEE"}; @@ -132,9 +132,39 @@ public void testNullBufferAggregate() String result = ((String) agg.get(buf, position)); + Assert.assertEquals(strings[0], result); + } + + @Test + public void testNullFirstBufferAggregate() + { + + final String[] strings = {null, "CCCC", "AAAA", "BBBB", "EEEE"}; + Integer maxStringBytes = 1024; + + TestObjectColumnSelector objectColumnSelector = new TestObjectColumnSelector<>(strings); - Assert.assertEquals("expected any string value", strings[0], result); + StringAnyAggregatorFactory factory = new StringAnyAggregatorFactory( + "billy", "billy", maxStringBytes + ); + + StringAnyBufferAggregator agg = new StringAnyBufferAggregator( + objectColumnSelector, + maxStringBytes + ); + + ByteBuffer buf = ByteBuffer.allocate(factory.getMaxIntermediateSize()); + int position = 0; + + agg.init(buf, position); + //noinspection ForLoopReplaceableByForEach + for (int i = 0; i < strings.length; i++) { + aggregateBuffer(objectColumnSelector, agg, buf, position); + } + + String result = ((String) agg.get(buf, position)); + Assert.assertNull(result); } @Test From 094425a6605b2646c1ac87d03130009ddac05cec Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 11 Feb 2020 18:08:32 -0800 Subject: [PATCH 6/9] address review comments --- docs/querying/aggregations.md | 2 +- docs/querying/sql.md | 2 +- .../aggregation/any/DoubleAnyAggregator.java | 4 --- .../any/DoubleAnyAggregatorFactory.java | 4 +-- .../any/DoubleAnyBufferAggregator.java | 16 +++++------- .../aggregation/any/FloatAnyAggregator.java | 4 --- .../any/FloatAnyAggregatorFactory.java | 12 +-------- .../any/FloatAnyBufferAggregator.java | 9 ------- .../aggregation/any/LongAnyAggregator.java | 8 ------ .../any/LongAnyAggregatorFactory.java | 12 +-------- .../any/LongAnyBufferAggregator.java | 9 ------- .../any/NumericAnyBufferAggregator.java | 26 +++++++------------ .../any/StringAnyBufferAggregator.java | 25 +++++++++--------- .../apache/druid/server/QueryLifecycle.java | 8 +++--- .../druid/sql/calcite/CalciteQueryTest.java | 1 - 15 files changed, 38 insertions(+), 104 deletions(-) diff --git a/docs/querying/aggregations.md b/docs/querying/aggregations.md index 024a43b546fc..a765b797e4c8 100644 --- a/docs/querying/aggregations.md +++ b/docs/querying/aggregations.md @@ -238,7 +238,7 @@ Note that queries with first/last aggregators on a segment created with rollup e (Double/Float/Long/String) ANY aggregator cannot be used in ingestion spec, and should only be specified as part of queries. -If `druid.generic.useDefaultValueForNull=true` aggregation can returns the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then aggregation will returns any value including null. +Returns any value including null. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null) #### `doubleAny` aggregator diff --git a/docs/querying/sql.md b/docs/querying/sql.md index 29bad8c477a5..d82762b46cd3 100644 --- a/docs/querying/sql.md +++ b/docs/querying/sql.md @@ -205,7 +205,7 @@ Only the COUNT aggregation can accept DISTINCT. |`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| |`LATEST(expr)`|Returns the latest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "latest" is the value last encountered with the maximum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the last value encountered.| |`LATEST(expr, maxBytesPerString)`|Like `LATEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| -|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any value of `expr` including null| +|`ANY_VALUE(expr)`|Returns any value of `expr` including null. `expr` must be numeric. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null)| |`ANY_VALUE(expr, maxBytesPerString)`|Like `ANY_VALUE(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java index fcc817ae9a9d..8b6a75d4a0f8 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java @@ -23,10 +23,6 @@ import javax.annotation.Nullable; -/** - * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class DoubleAnyAggregator extends NumericAnyAggregator { private double foundValue; diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java index 09d26812a4b6..99e96c556417 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java @@ -28,7 +28,6 @@ import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.DoubleSumAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.ColumnSelectorFactory; @@ -190,14 +189,13 @@ public byte[] getCacheKey() @Override public String getTypeName() { - // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde return storeDoubleAsFloat ? "float" : "double"; } @Override public int getMaxIntermediateSize() { - return Double.BYTES + Byte.BYTES + Byte.BYTES; + return Double.BYTES + Byte.BYTES; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java index df77f5458f22..6737cd694d45 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyBufferAggregator.java @@ -24,10 +24,6 @@ import javax.annotation.Nullable; import java.nio.ByteBuffer; -/** - * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class DoubleAnyBufferAggregator extends NumericAnyBufferAggregator { public DoubleAnyBufferAggregator( @@ -40,13 +36,13 @@ public DoubleAnyBufferAggregator( @Override void initValue(ByteBuffer buf, int position) { - buf.putDouble(getFoundValueStoredPosition(position), 0); + buf.putDouble(position + FOUND_VALUE_OFFSET, 0); } @Override void putValue(ByteBuffer buf, int position) { - buf.putDouble(getFoundValueStoredPosition(position), valueSelector.getDouble()); + buf.putDouble(position + FOUND_VALUE_OFFSET, valueSelector.getDouble()); } @Override @@ -54,24 +50,24 @@ void putValue(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { final boolean isNull = isValueNull(buf, position); - return isNull ? null : buf.getDouble(getFoundValueStoredPosition(position)); + return isNull ? null : buf.getDouble(position + FOUND_VALUE_OFFSET); } @Override public float getFloat(ByteBuffer buf, int position) { - return (float) buf.getDouble(getFoundValueStoredPosition(position)); + return (float) buf.getDouble(position + FOUND_VALUE_OFFSET); } @Override public long getLong(ByteBuffer buf, int position) { - return (long) buf.getDouble(getFoundValueStoredPosition(position)); + return (long) buf.getDouble(position + FOUND_VALUE_OFFSET); } @Override public double getDouble(ByteBuffer buf, int position) { - return buf.getDouble(getFoundValueStoredPosition(position)); + return buf.getDouble(position + FOUND_VALUE_OFFSET); } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java index 4e6aae55a192..c18eaf3d864a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregator.java @@ -23,10 +23,6 @@ import javax.annotation.Nullable; -/** - * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class FloatAnyAggregator extends NumericAnyAggregator { private float foundValue; diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java index fbf7ab88c79a..2ecfc46ba841 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java @@ -19,31 +19,22 @@ package org.apache.druid.query.aggregation.any; -import com.fasterxml.jackson.annotation.JacksonInject; 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.UOE; -import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.aggregation.AggregateCombiner; import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.FloatSumAggregator; -import org.apache.druid.query.aggregation.SimpleFloatAggregatorFactory; -import org.apache.druid.query.aggregation.first.FloatFirstAggregator; -import org.apache.druid.query.aggregation.first.FloatFirstAggregatorFactory; -import org.apache.druid.query.aggregation.first.FloatFirstBufferAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseFloatColumnValueSelector; import org.apache.druid.segment.ColumnSelectorFactory; import org.apache.druid.segment.NilColumnValueSelector; -import org.apache.druid.segment.column.ColumnHolder; import javax.annotation.Nullable; import java.nio.ByteBuffer; -import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -196,14 +187,13 @@ public byte[] getCacheKey() @Override public String getTypeName() { - // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde return "float"; } @Override public int getMaxIntermediateSize() { - return Float.BYTES + Byte.BYTES + Byte.BYTES; + return Float.BYTES + Byte.BYTES; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java index cb90c63c1ca8..d92072bfd1c0 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java @@ -19,20 +19,11 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; -import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; -import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseFloatColumnValueSelector; import javax.annotation.Nullable; import java.nio.ByteBuffer; -/** - * This Aggregator is created by the {@link FloatAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class FloatAnyBufferAggregator extends NumericAnyBufferAggregator { public FloatAnyBufferAggregator( diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java index d84855fbc0f8..28cdfe52e46a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregator.java @@ -19,18 +19,10 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.query.aggregation.Aggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; -import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseLongColumnValueSelector; import javax.annotation.Nullable; -/** - * This Aggregator is created by the {@link LongAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class LongAnyAggregator extends NumericAnyAggregator { private long foundValue; diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java index 3d8d264aca1a..6add01fb4dea 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java @@ -19,31 +19,22 @@ package org.apache.druid.query.aggregation.any; -import com.fasterxml.jackson.annotation.JacksonInject; 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.UOE; -import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.aggregation.AggregateCombiner; import org.apache.druid.query.aggregation.Aggregator; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.AggregatorUtil; import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.LongSumAggregator; -import org.apache.druid.query.aggregation.SimpleLongAggregatorFactory; -import org.apache.druid.query.aggregation.first.LongFirstAggregator; -import org.apache.druid.query.aggregation.first.LongFirstAggregatorFactory; -import org.apache.druid.query.aggregation.first.LongFirstBufferAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.BaseLongColumnValueSelector; import org.apache.druid.segment.ColumnSelectorFactory; import org.apache.druid.segment.NilColumnValueSelector; -import org.apache.druid.segment.column.ColumnHolder; import javax.annotation.Nullable; import java.nio.ByteBuffer; -import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -194,14 +185,13 @@ public byte[] getCacheKey() @Override public String getTypeName() { - // if we don't pretend to be a primitive, group by v1 gets sad and doesn't work because no complex type serde return "long"; } @Override public int getMaxIntermediateSize() { - return Long.BYTES + Byte.BYTES + Byte.BYTES; + return Long.BYTES + Byte.BYTES; } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java index c8f0b7a8085b..bddcc0456fdf 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java @@ -19,20 +19,11 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.query.aggregation.BufferAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregator; -import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory; -import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; -import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.BaseLongColumnValueSelector; import javax.annotation.Nullable; import java.nio.ByteBuffer; -/** - * This Aggregator is created by the {@link LongAnyAggregatorFactory} which has no special null handling logic. - * Hence, null can be pass into this aggregator from the valueSelector and null can be return from this aggregator. - */ public class LongAnyBufferAggregator extends NumericAnyBufferAggregator { public LongAnyBufferAggregator( diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java index f4a064464d58..56e5606b4169 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java @@ -32,11 +32,11 @@ public abstract class NumericAnyBufferAggregator implements BufferAggregator { - private static final byte BYTE_FLAG_IS_NOT_SET = 0; - private static final byte BYTE_FLAG_IS_SET = 1; - private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0; - private static final int IS_NULL_FLAG_OFFSET_POSITION = IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES; - private static final int FOUND_VALUE_OFFSET_POSITION = IS_NULL_FLAG_OFFSET_POSITION + Byte.BYTES; + // Rightmost bit for is null check (0 for is null and 1 for not null) + // Second rightmost bit for is found check (0 for not found and 1 for found) + private static final byte BYTE_FLAG_FOUND_MASK = 0b0010; + private static final byte BYTE_FLAG_NULL_MASK = 0b0001; + static final int FOUND_VALUE_OFFSET = Byte.BYTES; private final boolean useDefault = NullHandling.replaceWithDefault(); @@ -61,32 +61,26 @@ public NumericAnyBufferAggregator(TSelector valueSelector) @Override public void init(ByteBuffer buf, int position) { - buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); - buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, useDefault ? BYTE_FLAG_IS_NOT_SET : BYTE_FLAG_IS_SET); + buf.put(position, useDefault ? NullHandling.IS_NOT_NULL_BYTE : NullHandling.IS_NULL_BYTE); initValue(buf, position); } @Override public void aggregate(ByteBuffer buf, int position) { - if (buf.get(position + IS_FOUND_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_NOT_SET) { + if ((buf.get(position) & BYTE_FLAG_FOUND_MASK) != BYTE_FLAG_FOUND_MASK) { if (useDefault || !valueSelector.isNull()) { putValue(buf, position); - buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); + buf.put(position, (byte) (BYTE_FLAG_FOUND_MASK | NullHandling.IS_NOT_NULL_BYTE)); } else { - buf.put(position + IS_NULL_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); + buf.put(position, (byte) (BYTE_FLAG_FOUND_MASK | NullHandling.IS_NULL_BYTE)); } - buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); } } boolean isValueNull(ByteBuffer buf, int position) { - return buf.get(position + IS_NULL_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_SET; - } - - int getFoundValueStoredPosition(int position) { - return position + FOUND_VALUE_OFFSET_POSITION; + return (buf.get(position) & BYTE_FLAG_NULL_MASK) == NullHandling.IS_NULL_BYTE; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java index 716f38011410..b1bcc0aeffbf 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java @@ -19,7 +19,6 @@ package org.apache.druid.query.aggregation.any; -import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.aggregation.BufferAggregator; import org.apache.druid.segment.BaseObjectColumnValueSelector; @@ -32,9 +31,9 @@ public class StringAnyBufferAggregator implements BufferAggregator private static final byte BYTE_FLAG_IS_NOT_SET = 0; private static final byte BYTE_FLAG_IS_SET = 1; private static final int NULL_STRING_LENGTH = -1; - private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0; - private static final int STRING_LENGTH_OFFSET_POSITION = IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES; - private static final int FOUND_VALUE_OFFSET_POSITION = STRING_LENGTH_OFFSET_POSITION + Integer.BYTES; + private static final int IS_FOUND_FLAG_OFFSET = 0; + private static final int STRING_LENGTH_OFFSET = IS_FOUND_FLAG_OFFSET + Byte.BYTES; + private static final int FOUND_VALUE_OFFSET = STRING_LENGTH_OFFSET + Integer.BYTES; private final BaseObjectColumnValueSelector valueSelector; private final int maxStringBytes; @@ -48,26 +47,26 @@ public StringAnyBufferAggregator(BaseObjectColumnValueSelector valueSelector, in @Override public void init(ByteBuffer buf, int position) { - buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_NOT_SET); - buf.putInt(position + STRING_LENGTH_OFFSET_POSITION, NULL_STRING_LENGTH); + buf.put(position + IS_FOUND_FLAG_OFFSET, BYTE_FLAG_IS_NOT_SET); + buf.putInt(position + STRING_LENGTH_OFFSET, NULL_STRING_LENGTH); } @Override public void aggregate(ByteBuffer buf, int position) { - if (buf.get(position + IS_FOUND_FLAG_OFFSET_POSITION) == BYTE_FLAG_IS_NOT_SET) { + if (buf.get(position + IS_FOUND_FLAG_OFFSET) == BYTE_FLAG_IS_NOT_SET) { final Object object = valueSelector.getObject(); String foundValue = DimensionHandlerUtils.convertObjectToString(object); if (foundValue != null) { ByteBuffer mutationBuffer = buf.duplicate(); - mutationBuffer.position(position + FOUND_VALUE_OFFSET_POSITION); - mutationBuffer.limit(position + FOUND_VALUE_OFFSET_POSITION + maxStringBytes); + mutationBuffer.position(position + FOUND_VALUE_OFFSET); + mutationBuffer.limit(position + FOUND_VALUE_OFFSET + maxStringBytes); final int len = StringUtils.toUtf8WithLimit(foundValue, mutationBuffer); - mutationBuffer.putInt(position + STRING_LENGTH_OFFSET_POSITION, len); + mutationBuffer.putInt(position + STRING_LENGTH_OFFSET, len); } else { - buf.putInt(position + STRING_LENGTH_OFFSET_POSITION, NULL_STRING_LENGTH); + buf.putInt(position + STRING_LENGTH_OFFSET, NULL_STRING_LENGTH); } - buf.put(position + IS_FOUND_FLAG_OFFSET_POSITION, BYTE_FLAG_IS_SET); + buf.put(position + IS_FOUND_FLAG_OFFSET, BYTE_FLAG_IS_SET); } } @@ -75,7 +74,7 @@ public void aggregate(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { ByteBuffer copyBuffer = buf.duplicate(); - copyBuffer.position(position + STRING_LENGTH_OFFSET_POSITION); + copyBuffer.position(position + STRING_LENGTH_OFFSET); int stringSizeBytes = copyBuffer.getInt(); if (stringSizeBytes >= 0) { byte[] valueBytes = new byte[stringSizeBytes]; diff --git a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java index 7b60188c9e44..ff04a58fcd36 100644 --- a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java +++ b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java @@ -318,10 +318,12 @@ public void emitLogsAndMetrics( if (e != null) { statsMap.put("exception", e.toString()); - // Mimic behavior from QueryResource, where this code was originally taken from. log.noStackTrace().warn(e, "Exception while processing queryId [%s]", baseQuery.getId()); - statsMap.put("interrupted", e instanceof QueryInterruptedException); - statsMap.put("reason", e.toString()); + if (e instanceof QueryInterruptedException) { + // Mimic behavior from QueryResource, where this code was originally taken from. + statsMap.put("interrupted", true); + statsMap.put("reason", e.toString()); + } } requestLogger.logNativeQuery( diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index ed39d29ecf8c..39dd06b3faaa 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -21,7 +21,6 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.tools.ValidationException; import org.apache.druid.common.config.NullHandling; From 00eb20ed80c3041d688818468cd0c15d3ba91663 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 11 Feb 2020 18:21:16 -0800 Subject: [PATCH 7/9] optimize StringAnyBufferAggregator --- .../any/StringAnyAggregatorFactory.java | 2 +- .../any/StringAnyBufferAggregator.java | 21 +++++++------------ 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java index 1eb9d7a9f259..9e28b084bb61 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyAggregatorFactory.java @@ -155,7 +155,7 @@ public String getTypeName() @Override public int getMaxIntermediateSize() { - return Byte.BYTES + Integer.BYTES + maxStringBytes; + return Integer.BYTES + maxStringBytes; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java index b1bcc0aeffbf..4005a031f345 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java @@ -28,12 +28,9 @@ public class StringAnyBufferAggregator implements BufferAggregator { - private static final byte BYTE_FLAG_IS_NOT_SET = 0; - private static final byte BYTE_FLAG_IS_SET = 1; - private static final int NULL_STRING_LENGTH = -1; - private static final int IS_FOUND_FLAG_OFFSET = 0; - private static final int STRING_LENGTH_OFFSET = IS_FOUND_FLAG_OFFSET + Byte.BYTES; - private static final int FOUND_VALUE_OFFSET = STRING_LENGTH_OFFSET + Integer.BYTES; + private static final int FOUND_AND_NULL_FLAG_VALUE = -1; + private static final int NOT_FOUND_FLAG_VALUE = -2; + private static final int FOUND_VALUE_OFFSET = Integer.BYTES; private final BaseObjectColumnValueSelector valueSelector; private final int maxStringBytes; @@ -47,14 +44,13 @@ public StringAnyBufferAggregator(BaseObjectColumnValueSelector valueSelector, in @Override public void init(ByteBuffer buf, int position) { - buf.put(position + IS_FOUND_FLAG_OFFSET, BYTE_FLAG_IS_NOT_SET); - buf.putInt(position + STRING_LENGTH_OFFSET, NULL_STRING_LENGTH); + buf.putInt(position, NOT_FOUND_FLAG_VALUE); } @Override public void aggregate(ByteBuffer buf, int position) { - if (buf.get(position + IS_FOUND_FLAG_OFFSET) == BYTE_FLAG_IS_NOT_SET) { + if (buf.get(position) == NOT_FOUND_FLAG_VALUE) { final Object object = valueSelector.getObject(); String foundValue = DimensionHandlerUtils.convertObjectToString(object); if (foundValue != null) { @@ -62,11 +58,10 @@ public void aggregate(ByteBuffer buf, int position) mutationBuffer.position(position + FOUND_VALUE_OFFSET); mutationBuffer.limit(position + FOUND_VALUE_OFFSET + maxStringBytes); final int len = StringUtils.toUtf8WithLimit(foundValue, mutationBuffer); - mutationBuffer.putInt(position + STRING_LENGTH_OFFSET, len); + mutationBuffer.putInt(position, len); } else { - buf.putInt(position + STRING_LENGTH_OFFSET, NULL_STRING_LENGTH); + buf.putInt(position, FOUND_AND_NULL_FLAG_VALUE); } - buf.put(position + IS_FOUND_FLAG_OFFSET, BYTE_FLAG_IS_SET); } } @@ -74,7 +69,7 @@ public void aggregate(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { ByteBuffer copyBuffer = buf.duplicate(); - copyBuffer.position(position + STRING_LENGTH_OFFSET); + copyBuffer.position(position); int stringSizeBytes = copyBuffer.getInt(); if (stringSizeBytes >= 0) { byte[] valueBytes = new byte[stringSizeBytes]; From b9ef751385492faca5d57fbf05d0c7a71a1ff7e6 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 12 Feb 2020 00:14:24 -0800 Subject: [PATCH 8/9] fix failing tests --- .../aggregation/any/FloatAnyBufferAggregator.java | 12 ++++++------ .../aggregation/any/LongAnyBufferAggregator.java | 12 ++++++------ .../aggregation/any/StringAnyBufferAggregator.java | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java index d92072bfd1c0..b924a7a3252d 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyBufferAggregator.java @@ -37,13 +37,13 @@ public FloatAnyBufferAggregator( @Override void initValue(ByteBuffer buf, int position) { - buf.putFloat(getFoundValueStoredPosition(position), 0); + buf.putFloat(position + FOUND_VALUE_OFFSET, 0); } @Override void putValue(ByteBuffer buf, int position) { - buf.putFloat(getFoundValueStoredPosition(position), valueSelector.getFloat()); + buf.putFloat(position + FOUND_VALUE_OFFSET, valueSelector.getFloat()); } @Override @@ -51,25 +51,25 @@ void putValue(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { final boolean isNull = isValueNull(buf, position); - return isNull ? null : buf.getFloat(getFoundValueStoredPosition(position)); + return isNull ? null : buf.getFloat(position + FOUND_VALUE_OFFSET); } @Override public float getFloat(ByteBuffer buf, int position) { - return buf.getFloat(getFoundValueStoredPosition(position)); + return buf.getFloat(position + FOUND_VALUE_OFFSET); } @Override public long getLong(ByteBuffer buf, int position) { - return (long) buf.getFloat(getFoundValueStoredPosition(position)); + return (long) buf.getFloat(position + FOUND_VALUE_OFFSET); } @Override public double getDouble(ByteBuffer buf, int position) { - return (double) buf.getFloat(getFoundValueStoredPosition(position)); + return (double) buf.getFloat(position + FOUND_VALUE_OFFSET); } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java index bddcc0456fdf..91c1fbe03206 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyBufferAggregator.java @@ -36,13 +36,13 @@ public LongAnyBufferAggregator( @Override void initValue(ByteBuffer buf, int position) { - buf.putLong(getFoundValueStoredPosition(position), 0); + buf.putLong(position + FOUND_VALUE_OFFSET, 0); } @Override void putValue(ByteBuffer buf, int position) { - buf.putLong(getFoundValueStoredPosition(position), valueSelector.getLong()); + buf.putLong(position + FOUND_VALUE_OFFSET, valueSelector.getLong()); } @Override @@ -50,24 +50,24 @@ void putValue(ByteBuffer buf, int position) public Object get(ByteBuffer buf, int position) { final boolean isNull = isValueNull(buf, position); - return isNull ? null : buf.getLong(getFoundValueStoredPosition(position)); + return isNull ? null : buf.getLong(position + FOUND_VALUE_OFFSET); } @Override public float getFloat(ByteBuffer buf, int position) { - return (float) buf.getLong(getFoundValueStoredPosition(position)); + return (float) buf.getLong(position + FOUND_VALUE_OFFSET); } @Override public double getDouble(ByteBuffer buf, int position) { - return (double) buf.getLong(getFoundValueStoredPosition(position)); + return (double) buf.getLong(position + FOUND_VALUE_OFFSET); } @Override public long getLong(ByteBuffer buf, int position) { - return buf.getLong(getFoundValueStoredPosition(position)); + return buf.getLong(position + FOUND_VALUE_OFFSET); } } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java index 4005a031f345..32bb3153fa2b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/StringAnyBufferAggregator.java @@ -50,7 +50,7 @@ public void init(ByteBuffer buf, int position) @Override public void aggregate(ByteBuffer buf, int position) { - if (buf.get(position) == NOT_FOUND_FLAG_VALUE) { + if (buf.getInt(position) == NOT_FOUND_FLAG_VALUE) { final Object object = valueSelector.getObject(); String foundValue = DimensionHandlerUtils.convertObjectToString(object); if (foundValue != null) { From c3e645c75948c58a48767fd3e3904d784c1c7222 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Wed, 12 Feb 2020 10:20:10 -0800 Subject: [PATCH 9/9] address pr comments --- docs/querying/aggregations.md | 2 +- docs/querying/sql.md | 2 +- .../query/aggregation/any/NumericAnyBufferAggregator.java | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/querying/aggregations.md b/docs/querying/aggregations.md index a765b797e4c8..3531f2d95efa 100644 --- a/docs/querying/aggregations.md +++ b/docs/querying/aggregations.md @@ -238,7 +238,7 @@ Note that queries with first/last aggregators on a segment created with rollup e (Double/Float/Long/String) ANY aggregator cannot be used in ingestion spec, and should only be specified as part of queries. -Returns any value including null. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null) +Returns any value including null. This aggregator can simplify and optimize the performance by returning the first encountered value (including null) #### `doubleAny` aggregator diff --git a/docs/querying/sql.md b/docs/querying/sql.md index d82762b46cd3..6e9a230c6283 100644 --- a/docs/querying/sql.md +++ b/docs/querying/sql.md @@ -205,7 +205,7 @@ Only the COUNT aggregation can accept DISTINCT. |`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| |`LATEST(expr)`|Returns the latest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "latest" is the value last encountered with the maximum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the last value encountered.| |`LATEST(expr, maxBytesPerString)`|Like `LATEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| -|`ANY_VALUE(expr)`|Returns any value of `expr` including null. `expr` must be numeric. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null)| +|`ANY_VALUE(expr)`|Returns any value of `expr` including null. `expr` must be numeric. This aggregator can simplify and optimize the performance by returning the first encountered value (including null)| |`ANY_VALUE(expr, maxBytesPerString)`|Like `ANY_VALUE(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java index 56e5606b4169..d39b77419791 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java @@ -34,8 +34,8 @@ public abstract class NumericAnyBufferAggregator