diff --git a/extensions-contrib/time-min-max/pom.xml b/extensions-contrib/time-min-max/pom.xml index 07742ce2b1b..ff87ef2c7ca 100644 --- a/extensions-contrib/time-min-max/pom.xml +++ b/extensions-contrib/time-min-max/pom.xml @@ -54,6 +54,16 @@ jackson-annotations provided + + com.fasterxml.jackson.core + jackson-core + provided + + + com.fasterxml.jackson.core + jackson-databind + provided + com.google.guava guava @@ -69,11 +79,6 @@ guice provided - - com.fasterxml.jackson.core - jackson-databind - provided - it.unimi.dsi fastutil @@ -89,6 +94,11 @@ easymock test + + nl.jqno.equalsverifier + equalsverifier + test + org.apache.druid druid-core diff --git a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregator.java b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregator.java index c11b50dcece..d1e8dde1bdf 100644 --- a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregator.java +++ b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregator.java @@ -38,7 +38,6 @@ public class TimestampAggregator implements Aggregator } private final BaseObjectColumnValueSelector selector; - private final String name; private final TimestampSpec timestampSpec; private final Comparator comparator; private final Long initValue; @@ -46,14 +45,12 @@ public class TimestampAggregator implements Aggregator private long most; public TimestampAggregator( - String name, BaseObjectColumnValueSelector selector, TimestampSpec timestampSpec, Comparator comparator, Long initValue ) { - this.name = name; this.selector = selector; this.timestampSpec = timestampSpec; this.comparator = comparator; diff --git a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregatorFactory.java b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregatorFactory.java index 21f79244761..dca89e26dbb 100644 --- a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregatorFactory.java +++ b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampAggregatorFactory.java @@ -22,24 +22,25 @@ package org.apache.druid.query.aggregation; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.data.input.impl.TimestampSpec; import org.apache.druid.java.util.common.DateTimes; -import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.cache.CacheKeyBuilder; import org.apache.druid.segment.ColumnSelectorFactory; import org.apache.druid.segment.ColumnValueSelector; import org.apache.druid.segment.column.ValueType; import org.joda.time.DateTime; import javax.annotation.Nullable; -import java.nio.ByteBuffer; import java.sql.Timestamp; import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Objects; -public class TimestampAggregatorFactory extends AggregatorFactory +public abstract class TimestampAggregatorFactory extends AggregatorFactory { final String name; + @Nullable final String fieldName; + @Nullable final String timeFormat; private final Comparator comparator; private final Long initValue; @@ -48,8 +49,8 @@ public class TimestampAggregatorFactory extends AggregatorFactory TimestampAggregatorFactory( String name, - String fieldName, - String timeFormat, + @Nullable String fieldName, + @Nullable String timeFormat, Comparator comparator, Long initValue ) @@ -66,13 +67,23 @@ public class TimestampAggregatorFactory extends AggregatorFactory @Override public Aggregator factorize(ColumnSelectorFactory metricFactory) { - return new TimestampAggregator(name, metricFactory.makeColumnValueSelector(fieldName), timestampSpec, comparator, initValue); + return new TimestampAggregator( + metricFactory.makeColumnValueSelector(timestampSpec.getTimestampColumn()), + timestampSpec, + comparator, + initValue + ); } @Override public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory) { - return new TimestampBufferAggregator(metricFactory.makeColumnValueSelector(fieldName), timestampSpec, comparator, initValue); + return new TimestampBufferAggregator( + metricFactory.makeColumnValueSelector(timestampSpec.getTimestampColumn()), + timestampSpec, + comparator, + initValue + ); } @Override @@ -129,12 +140,6 @@ public class TimestampAggregatorFactory extends AggregatorFactory }; } - @Override - public AggregatorFactory getCombiningFactory() - { - return new TimestampAggregatorFactory(name, name, timeFormat, comparator, initValue); - } - @Override public AggregatorFactory getMergingFactory(AggregatorFactory other) throws AggregatorFactoryNotMergeableException { @@ -145,14 +150,6 @@ public class TimestampAggregatorFactory extends AggregatorFactory } } - @Override - public List getRequiredColumns() - { - return Collections.singletonList( - new TimestampAggregatorFactory(fieldName, fieldName, timeFormat, comparator, initValue) - ); - } - @Override public Object deserialize(Object object) { @@ -173,12 +170,14 @@ public class TimestampAggregatorFactory extends AggregatorFactory return name; } + @Nullable @JsonProperty public String getFieldName() { return fieldName; } + @Nullable @JsonProperty public String getTimeFormat() { @@ -188,16 +187,15 @@ public class TimestampAggregatorFactory extends AggregatorFactory @Override public List requiredFields() { - return Collections.singletonList(fieldName); + return Collections.singletonList(timestampSpec.getTimestampColumn()); } @Override public byte[] getCacheKey() { - byte[] fieldNameBytes = StringUtils.toUtf8(fieldName); - - return ByteBuffer.allocate(1 + fieldNameBytes.length) - .put(AggregatorUtil.TIMESTAMP_CACHE_TYPE_ID).put(fieldNameBytes).array(); + return new CacheKeyBuilder(AggregatorUtil.TIMESTAMP_CACHE_TYPE_ID).appendString(timestampSpec.getTimestampColumn()) + .appendString(timestampSpec.getTimestampFormat()) + .build(); } @Override @@ -221,43 +219,6 @@ public class TimestampAggregatorFactory extends AggregatorFactory return Long.BYTES; } - @Override - public boolean equals(Object o) - { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - - TimestampAggregatorFactory that = (TimestampAggregatorFactory) o; - - if (!Objects.equals(fieldName, that.fieldName)) { - return false; - } - if (!Objects.equals(name, that.name)) { - return false; - } - if (!Objects.equals(comparator, that.comparator)) { - return false; - } - if (!Objects.equals(initValue, that.initValue)) { - return false; - } - - return true; - } - - @Override - public int hashCode() - { - int result = fieldName != null ? fieldName.hashCode() : 0; - result = 31 * result + (name != null ? name.hashCode() : 0); - result = 31 * result + (comparator != null ? comparator.hashCode() : 0); - result = 31 * result + (initValue != null ? initValue.hashCode() : 0); - return result; - } @Nullable static Long convertLong(TimestampSpec timestampSpec, Object input) @@ -274,4 +235,26 @@ public class TimestampAggregatorFactory extends AggregatorFactory return null; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + TimestampAggregatorFactory that = (TimestampAggregatorFactory) o; + return name.equals(that.name) && Objects.equals(fieldName, that.fieldName) && Objects.equals( + timeFormat, + that.timeFormat + ) && comparator.equals(that.comparator) && initValue.equals(that.initValue); + } + + @Override + public int hashCode() + { + return Objects.hash(name, fieldName, timeFormat, comparator, initValue); + } } diff --git a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMaxAggregatorFactory.java b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMaxAggregatorFactory.java index 4514c28c632..fea1c85080d 100644 --- a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMaxAggregatorFactory.java +++ b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMaxAggregatorFactory.java @@ -22,28 +22,46 @@ package org.apache.druid.query.aggregation; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Ordering; +import javax.annotation.Nullable; +import java.util.List; + public class TimestampMaxAggregatorFactory extends TimestampAggregatorFactory { @JsonCreator public TimestampMaxAggregatorFactory( @JsonProperty("name") String name, - @JsonProperty("fieldName") String fieldName, - @JsonProperty("timeFormat") String timeFormat + @JsonProperty("fieldName") @Nullable String fieldName, + @JsonProperty("timeFormat") @Nullable String timeFormat ) { super(name, fieldName, timeFormat, Ordering.natural(), Long.MIN_VALUE); Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); - Preconditions.checkNotNull(fieldName, "Must have a valid, non-null fieldName"); + } + + @Override + public AggregatorFactory getCombiningFactory() + { + return new TimestampMaxAggregatorFactory(name, name, timeFormat); + } + + @Override + public List getRequiredColumns() + { + return ImmutableList.of( + new TimestampMaxAggregatorFactory(name, fieldName, timeFormat) + ); } @Override public String toString() { return "TimestampMaxAggregatorFactory{" + - "fieldName='" + fieldName + '\'' + - ", name='" + name + '\'' + - '}'; + "name='" + name + '\'' + + ", fieldName='" + fieldName + '\'' + + ", timeFormat='" + timeFormat + '\'' + + '}'; } } diff --git a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMinAggregatorFactory.java b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMinAggregatorFactory.java index 6e4a0ead048..79cb1cadfae 100644 --- a/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMinAggregatorFactory.java +++ b/extensions-contrib/time-min-max/src/main/java/org/apache/druid/query/aggregation/TimestampMinAggregatorFactory.java @@ -22,8 +22,11 @@ package org.apache.druid.query.aggregation; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Ordering; +import java.util.List; + public class TimestampMinAggregatorFactory extends TimestampAggregatorFactory { @JsonCreator @@ -35,15 +38,29 @@ public class TimestampMinAggregatorFactory extends TimestampAggregatorFactory { super(name, fieldName, timeFormat, Ordering.natural().reverse(), Long.MAX_VALUE); Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); - Preconditions.checkNotNull(fieldName, "Must have a valid, non-null fieldName"); + } + + @Override + public AggregatorFactory getCombiningFactory() + { + return new TimestampMinAggregatorFactory(name, name, timeFormat); + } + + @Override + public List getRequiredColumns() + { + return ImmutableList.of( + new TimestampMinAggregatorFactory(name, fieldName, timeFormat) + ); } @Override public String toString() { return "TimestampMinAggregatorFactory{" + - "fieldName='" + fieldName + '\'' + - ", name='" + name + '\'' + - '}'; + "name='" + name + '\'' + + ", fieldName='" + fieldName + '\'' + + ", timeFormat='" + timeFormat + '\'' + + '}'; } } diff --git a/extensions-contrib/time-min-max/src/test/java/org/apache/druid/query/aggregation/TimestampMinMaxAggregatorFactoryTest.java b/extensions-contrib/time-min-max/src/test/java/org/apache/druid/query/aggregation/TimestampMinMaxAggregatorFactoryTest.java index c8424c3319f..4b87f804ed3 100644 --- a/extensions-contrib/time-min-max/src/test/java/org/apache/druid/query/aggregation/TimestampMinMaxAggregatorFactoryTest.java +++ b/extensions-contrib/time-min-max/src/test/java/org/apache/druid/query/aggregation/TimestampMinMaxAggregatorFactoryTest.java @@ -19,12 +19,16 @@ package org.apache.druid.query.aggregation; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import nl.jqno.equalsverifier.EqualsVerifier; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.query.Druids; import org.apache.druid.query.aggregation.post.FieldAccessPostAggregator; import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator; import org.apache.druid.query.timeseries.TimeseriesQuery; import org.apache.druid.query.timeseries.TimeseriesQueryQueryToolChest; +import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.segment.column.ValueType; import org.junit.Assert; @@ -32,6 +36,54 @@ import org.junit.Test; public class TimestampMinMaxAggregatorFactoryTest { + private static final ObjectMapper JSON_MAPPER = TestHelper.makeJsonMapper(); + + @Test + public void testSerde() throws JsonProcessingException + { + TimestampMaxAggregatorFactory maxAgg = new TimestampMaxAggregatorFactory("timeMax", "__time", null); + TimestampMinAggregatorFactory minAgg = new TimestampMinAggregatorFactory("timeMin", "__time", null); + + Assert.assertEquals( + maxAgg, + JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(maxAgg), TimestampMaxAggregatorFactory.class) + ); + Assert.assertEquals( + maxAgg.getCombiningFactory(), + JSON_MAPPER.readValue( + JSON_MAPPER.writeValueAsString(maxAgg.getCombiningFactory()), + TimestampMaxAggregatorFactory.class + ) + ); + + Assert.assertEquals( + minAgg, + JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(minAgg), TimestampMinAggregatorFactory.class) + ); + Assert.assertEquals( + minAgg.getCombiningFactory(), + JSON_MAPPER.readValue( + JSON_MAPPER.writeValueAsString(minAgg.getCombiningFactory()), + TimestampMinAggregatorFactory.class + ) + ); + } + + @Test + public void testEqualsAndHashcode() + { + EqualsVerifier.forClass(TimestampMinAggregatorFactory.class) + .withNonnullFields("name", "comparator", "initValue") + .withIgnoredFields("timestampSpec") + .usingGetClass() + .verify(); + EqualsVerifier.forClass(TimestampMaxAggregatorFactory.class) + .withNonnullFields("name", "comparator", "initValue") + .withIgnoredFields("timestampSpec") + .usingGetClass() + .verify(); + } + @Test public void testResultArraySignature() {