fix serde issues with time-min-max extension (#11146)

* fix serde issues with time-min-max extension

* fix pom dependencies

extensions-contrib/time-min-max/pom.xml

@@ -54,6 +54,16 @@
       <artifactId>jackson-annotations</artifactId>
       <scope>provided</scope>
     </dependency>
+    <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-core</artifactId>
+      <scope>provided</scope>
+    </dependency>
+    <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-databind</artifactId>
+      <scope>provided</scope>
+    </dependency>
     <dependency>
       <groupId>com.google.guava</groupId>
       <artifactId>guava</artifactId>
@@ -69,11 +79,6 @@
       <artifactId>guice</artifactId>
       <scope>provided</scope>
     </dependency>
-    <dependency>
-      <groupId>com.fasterxml.jackson.core</groupId>
-      <artifactId>jackson-databind</artifactId>
-      <scope>provided</scope>
-    </dependency>
     <dependency>
       <groupId>it.unimi.dsi</groupId>
       <artifactId>fastutil</artifactId>
@@ -89,6 +94,11 @@
       <artifactId>easymock</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>nl.jqno.equalsverifier</groupId>
+      <artifactId>equalsverifier</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.apache.druid</groupId>
       <artifactId>druid-core</artifactId>

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<Long> 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<Long> comparator,
       Long initValue
   )
   {
-    this.name = name;
     this.selector = selector;
     this.timestampSpec = timestampSpec;
     this.comparator = comparator;

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<Long> 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<Long> 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<AggregatorFactory> 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<String> 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);
+  }
 }

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<AggregatorFactory> 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 + '\'' +
            '}';
   }
 }

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<AggregatorFactory> 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 + '\'' +
            '}';
   }
 }

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()
   {