Merge pull request #1412 from pjain1/alphaNumericTopN_NPE_fix

NPE fix for TopN query with alphaNumericTopN metric spec

processing/src/main/java/io/druid/query/topn/AlphaNumericTopNMetricSpec.java

@@ -38,7 +38,11 @@ public class AlphaNumericTopNMetricSpec extends LexicographicTopNMetricSpec
     {
       int[] pos = {0, 0};
 
-      if (str1.length() == 0) {
+      if (str1 == null) {
+        return -1;
+      } else if (str2 == null) {
+        return 1;
+      } else if (str1.length() == 0) {
         return str2.length() == 0 ? 0 : -1;
       } else if (str2.length() == 0) {
         return 1;
@@ -179,15 +183,12 @@ public class AlphaNumericTopNMetricSpec extends LexicographicTopNMetricSpec
     }
   };
 
-  private final String previousStop;
-
   @JsonCreator
   public AlphaNumericTopNMetricSpec(
       @JsonProperty("previousStop") String previousStop
   )
   {
     super(previousStop);
-    this.previousStop = (previousStop == null) ? "" : previousStop;
   }
 
   @Override
@@ -199,7 +200,7 @@ public class AlphaNumericTopNMetricSpec extends LexicographicTopNMetricSpec
   @Override
   public byte[] getCacheKey()
   {
-    byte[] previousStopBytes = StringUtils.toUtf8(previousStop);
+    byte[] previousStopBytes = getPreviousStop() == null ? new byte[]{} : StringUtils.toUtf8(getPreviousStop());
 
     return ByteBuffer.allocate(1 + previousStopBytes.length)
                      .put(CACHE_TYPE_ID)
@@ -217,7 +218,7 @@ public class AlphaNumericTopNMetricSpec extends LexicographicTopNMetricSpec
   public String toString()
   {
     return "AlphaNumericTopNMetricSpec{" +
-           "previousStop='" + previousStop + '\'' +
+           "previousStop='" + getPreviousStop() + '\'' +
            '}';
   }
 }

processing/src/test/java/io/druid/query/topn/AlphaNumericTopNMetricSpecTest.java

@@ -17,11 +17,15 @@
 
 package io.druid.query.topn;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+import io.druid.jackson.DefaultObjectMapper;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.util.Comparator;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class AlphaNumericTopNMetricSpecTest
 {
@@ -60,4 +64,22 @@ public class AlphaNumericTopNMetricSpecTest
     assertTrue(comparator.compare("1.3", "1.15") < 0);
 
   }
+
+  @Test
+  public void testSerdeAlphaNumericTopNMetricSpec() throws IOException{
+    AlphaNumericTopNMetricSpec expectedMetricSpec = new AlphaNumericTopNMetricSpec(null);
+    AlphaNumericTopNMetricSpec expectedMetricSpec1 = new AlphaNumericTopNMetricSpec("test");
+    String jsonSpec = "{\n"
+                      + "    \"type\": \"alphaNumeric\"\n"
+                      + "}";
+    String jsonSpec1 = "{\n"
+                       + "    \"type\": \"alphaNumeric\",\n"
+                       + "    \"previousStop\": \"test\"\n"
+                       + "}";
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    TopNMetricSpec actualMetricSpec = jsonMapper.readValue(jsonMapper.writeValueAsString(jsonMapper.readValue(jsonSpec, TopNMetricSpec.class)), AlphaNumericTopNMetricSpec.class);
+    TopNMetricSpec actualMetricSpec1 = jsonMapper.readValue(jsonMapper.writeValueAsString(jsonMapper.readValue(jsonSpec1, TopNMetricSpec.class)), AlphaNumericTopNMetricSpec.class);
+    assertEquals(expectedMetricSpec, actualMetricSpec);
+    assertEquals(expectedMetricSpec1, actualMetricSpec1);
+  }
 }

processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java

@@ -2991,4 +2991,34 @@ public class TopNQueryRunnerTest
     );
     assertExpectedResults(expectedResults, query);
   }
+  @Test
+  public void testAlphaNumericTopNWithNullPreviousStop(){
+    TopNQuery query = new TopNQueryBuilder()
+        .dataSource(QueryRunnerTestHelper.dataSource)
+        .granularity(QueryGranularity.ALL)
+        .dimension(QueryRunnerTestHelper.marketDimension)
+        .metric(new AlphaNumericTopNMetricSpec(null))
+        .threshold(2)
+        .intervals(QueryRunnerTestHelper.secondOnly)
+        .aggregators(Lists.<AggregatorFactory>newArrayList(QueryRunnerTestHelper.rowsCount))
+        .build();
+    List<Result<TopNResultValue>> expectedResults = Arrays.asList(
+        new Result<>(
+            new DateTime("2011-04-02T00:00:00.000Z"),
+            new TopNResultValue(
+                Arrays.asList(
+                    ImmutableMap.<String, Object>of(
+                        "market", "spot",
+                        "rows", 9L
+                    ),
+                    ImmutableMap.<String, Object>of(
+                        "market", "total_market",
+                        "rows", 2L
+                    )
+                )
+            )
+        )
+    );
+    TestHelper.assertExpectedResults(expectedResults, runner.run(query, new HashMap<String, Object>()));
+  }
 }

processing/src/test/java/io/druid/query/topn/TopNQueryTest.java

@@ -26,6 +26,7 @@ import io.druid.query.aggregation.AggregatorFactory;
 import io.druid.query.aggregation.DoubleMaxAggregatorFactory;
 import io.druid.query.aggregation.DoubleMinAggregatorFactory;
 import io.druid.query.aggregation.PostAggregator;
+import io.druid.query.dimension.LegacyDimensionSpec;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -39,6 +40,7 @@ import static io.druid.query.QueryRunnerTestHelper.dataSource;
 import static io.druid.query.QueryRunnerTestHelper.fullOnInterval;
 import static io.druid.query.QueryRunnerTestHelper.indexMetric;
 import static io.druid.query.QueryRunnerTestHelper.marketDimension;
+import static io.druid.query.QueryRunnerTestHelper.rowsCount;
 
 public class TopNQueryTest
 {
@@ -74,4 +76,38 @@ public class TopNQueryTest
     Assert.assertEquals(query, serdeQuery);
   }
 
+  @Test
+  public void testQuerySerdeWithAlphaNumericTopNMetricSpec() throws IOException{
+    TopNQuery expectedQuery = new TopNQueryBuilder()
+        .dataSource(dataSource)
+        .granularity(allGran)
+        .dimension(new LegacyDimensionSpec(marketDimension))
+        .metric(new AlphaNumericTopNMetricSpec(null))
+        .threshold(2)
+        .intervals(fullOnInterval.getIntervals())
+        .aggregators(Lists.<AggregatorFactory>newArrayList(rowsCount))
+        .build();
+    String jsonQuery = "{\n"
+                       + "  \"queryType\": \"topN\",\n"
+                       + "  \"dataSource\": \"testing\",\n"
+                       + "  \"dimension\": \"market\",\n"
+                       + "  \"threshold\": 2,\n"
+                       + "  \"metric\": {\n"
+                       + "    \"type\": \"alphaNumeric\"\n"
+                       + "   },\n"
+                       + "  \"granularity\": \"all\",\n"
+                       + "  \"aggregations\": [\n"
+                       + "    {\n"
+                       + "      \"type\": \"count\",\n"
+                       + "      \"name\": \"rows\"\n"
+                       + "    }\n"
+                       + "  ],\n"
+                       + "  \"intervals\": [\n"
+                       + "    \"1970-01-01T00:00:00.000Z/2020-01-01T00:00:00.000Z\"\n"
+                       + "  ]\n"
+                       + "}";
+    TopNQuery actualQuery = jsonMapper.readValue(jsonMapper.writeValueAsString(jsonMapper.readValue(jsonQuery, TopNQuery.class)), TopNQuery.class);
+    Assert.assertEquals(expectedQuery, actualQuery);
+  }
+
 }