Merge pull request #42 from metamx/fjy

bug fixes for invalid query params

client/src/main/java/com/metamx/druid/client/CachingClusteredClient.java

@@ -24,6 +24,7 @@ import com.google.common.base.Supplier;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Iterators;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Ordering;
@@ -237,6 +238,10 @@ public class CachingClusteredClient<T> implements QueryRunner<T>
                     public Iterator<Object> make()
                     {
                       try {
+                        if (cachedResult.length == 0) {
+                          return Iterators.emptyIterator();
+                        }
+
                         return objectMapper.readValues(
                             objectMapper.getJsonFactory().createJsonParser(cachedResult), Object.class
                         );

client/src/main/java/com/metamx/druid/query/Queries.java

@@ -0,0 +1,50 @@
+package com.metamx.druid.query;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.metamx.druid.aggregation.AggregatorFactory;
+import com.metamx.druid.aggregation.post.PostAggregator;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.Set;
+
+/**
+ */
+public class Queries
+{
+  public static void verifyAggregations(
+      List<AggregatorFactory> aggFactories,
+      List<PostAggregator> postAggs
+  )
+  {
+    Preconditions.checkNotNull(aggFactories, "aggregations cannot be null");
+    Preconditions.checkArgument(aggFactories.size() > 0, "Must have at least one AggregatorFactory");
+
+    if (postAggs != null && !postAggs.isEmpty()) {
+      Set<String> combinedAggNames = Sets.newHashSet(
+          Lists.transform(
+              aggFactories,
+              new Function<AggregatorFactory, String>()
+              {
+                @Override
+                public String apply(@Nullable AggregatorFactory input)
+                {
+                  return input.getName();
+                }
+              }
+          )
+      );
+
+      for (PostAggregator postAgg : postAggs) {
+        Preconditions.checkArgument(
+            postAgg.verifyFields(combinedAggNames),
+            String.format("Missing field[%s]", postAgg.getName())
+        );
+        combinedAggNames.add(postAgg.getName());
+      }
+    }
+  }
+}

client/src/main/java/com/metamx/druid/query/timeboundary/TimeBoundaryQuery.java

@@ -42,7 +42,10 @@ import java.util.Map;
  */
 public class TimeBoundaryQuery extends BaseQuery<Result<TimeBoundaryResultValue>>
 {
-  public static final Interval MY_Y2K_INTERVAL = new Interval(new DateTime(Long.MIN_VALUE), new DateTime(Long.MAX_VALUE));
+  public static final Interval MY_Y2K_INTERVAL = new Interval(
+      new DateTime(Long.MIN_VALUE),
+      new DateTime(Long.MAX_VALUE)
+  );
   public static final String MAX_TIME = "maxTime";
   public static final String MIN_TIME = "minTime";
   private static final byte CACHE_TYPE_ID = 0x0;
@@ -131,6 +134,10 @@ public class TimeBoundaryQuery extends BaseQuery<Result<TimeBoundaryResultValue>
 
   public Iterable<Result<TimeBoundaryResultValue>> mergeResults(List<Result<TimeBoundaryResultValue>> results)
   {
+    if (results == null || results.isEmpty()) {
+      return Lists.newArrayList();
+    }
+
     DateTime min = new DateTime(Long.MAX_VALUE);
     DateTime max = new DateTime(Long.MIN_VALUE);
     for (Result<TimeBoundaryResultValue> result : results) {

client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQuery.java

@@ -26,6 +26,7 @@ import com.metamx.druid.Query;
 import com.metamx.druid.QueryGranularity;
 import com.metamx.druid.aggregation.AggregatorFactory;
 import com.metamx.druid.aggregation.post.PostAggregator;
+import com.metamx.druid.query.Queries;
 import com.metamx.druid.query.filter.DimFilter;
 import com.metamx.druid.query.segment.QuerySegmentSpec;
 import com.metamx.druid.result.Result;
@@ -67,8 +68,7 @@ public class TimeseriesQuery extends BaseQuery<Result<TimeseriesResultValue>>
     this.aggregatorSpecs = aggregatorSpecs;
     this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.<PostAggregator>of() : postAggregatorSpecs;
 
-    Preconditions.checkNotNull(aggregatorSpecs, "aggregations cannot be null");
+    Queries.verifyAggregations(this.aggregatorSpecs, this.postAggregatorSpecs);
-    Preconditions.checkArgument(aggregatorSpecs.size() > 0, "Must have at least one AggregatorFactory");
   }
 
   @Override

client/src/test/java/com/metamx/druid/query/QueriesTest.java

@@ -0,0 +1,191 @@
+package com.metamx.druid.query;
+
+import com.metamx.druid.aggregation.AggregatorFactory;
+import com.metamx.druid.aggregation.CountAggregatorFactory;
+import com.metamx.druid.aggregation.DoubleSumAggregatorFactory;
+import com.metamx.druid.aggregation.post.ArithmeticPostAggregator;
+import com.metamx.druid.aggregation.post.ConstantPostAggregator;
+import com.metamx.druid.aggregation.post.FieldAccessPostAggregator;
+import com.metamx.druid.aggregation.post.PostAggregator;
+import junit.framework.Assert;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ */
+public class QueriesTest
+{
+  @Test
+  public void testVerifyAggregations() throws Exception
+  {
+    List<AggregatorFactory> aggFactories = Arrays.<AggregatorFactory>asList(
+        new CountAggregatorFactory("count"),
+        new DoubleSumAggregatorFactory("idx", "index"),
+        new DoubleSumAggregatorFactory("rev", "revenue")
+    );
+
+    List<PostAggregator> postAggs = Arrays.<PostAggregator>asList(
+        new ArithmeticPostAggregator(
+            "addStuff",
+            "+",
+            Arrays.<PostAggregator>asList(
+                new FieldAccessPostAggregator("idx", "idx"),
+                new FieldAccessPostAggregator("count", "count")
+            )
+        )
+    );
+
+    boolean exceptionOccured = false;
+
+    try {
+      Queries.verifyAggregations(aggFactories, postAggs);
+    }
+    catch (IllegalArgumentException e) {
+      exceptionOccured = true;
+    }
+
+    Assert.assertFalse(exceptionOccured);
+  }
+
+  @Test
+  public void testVerifyAggregationsMissingVal() throws Exception
+  {
+    List<AggregatorFactory> aggFactories = Arrays.<AggregatorFactory>asList(
+        new CountAggregatorFactory("count"),
+        new DoubleSumAggregatorFactory("idx", "index"),
+        new DoubleSumAggregatorFactory("rev", "revenue")
+    );
+
+    List<PostAggregator> postAggs = Arrays.<PostAggregator>asList(
+        new ArithmeticPostAggregator(
+            "addStuff",
+            "+",
+            Arrays.<PostAggregator>asList(
+                new FieldAccessPostAggregator("idx", "idx2"),
+                new FieldAccessPostAggregator("count", "count")
+            )
+        )
+    );
+
+    boolean exceptionOccured = false;
+
+    try {
+      Queries.verifyAggregations(aggFactories, postAggs);
+    }
+    catch (IllegalArgumentException e) {
+      exceptionOccured = true;
+    }
+
+    Assert.assertTrue(exceptionOccured);
+  }
+
+  @Test
+  public void testVerifyAggregationsMultiLevel() throws Exception
+  {
+    List<AggregatorFactory> aggFactories = Arrays.<AggregatorFactory>asList(
+        new CountAggregatorFactory("count"),
+        new DoubleSumAggregatorFactory("idx", "index"),
+        new DoubleSumAggregatorFactory("rev", "revenue")
+    );
+
+    List<PostAggregator> postAggs = Arrays.<PostAggregator>asList(
+        new ArithmeticPostAggregator(
+            "divideStuff",
+            "/",
+            Arrays.<PostAggregator>asList(
+                new ArithmeticPostAggregator(
+                    "addStuff",
+                    "+",
+                    Arrays.asList(
+                        new FieldAccessPostAggregator("idx", "idx"),
+                        new ConstantPostAggregator("const", 1)
+                    )
+                ),
+                new ArithmeticPostAggregator(
+                    "subtractStuff",
+                    "-",
+                    Arrays.asList(
+                        new FieldAccessPostAggregator("rev", "rev"),
+                        new ConstantPostAggregator("const", 1)
+                    )
+                )
+            )
+        ),
+        new ArithmeticPostAggregator(
+            "addStuff",
+            "+",
+            Arrays.<PostAggregator>asList(
+                new FieldAccessPostAggregator("divideStuff", "divideStuff"),
+                new FieldAccessPostAggregator("count", "count")
+            )
+        )
+    );
+
+    boolean exceptionOccured = false;
+
+    try {
+      Queries.verifyAggregations(aggFactories, postAggs);
+    }
+    catch (IllegalArgumentException e) {
+      exceptionOccured = true;
+    }
+
+    Assert.assertFalse(exceptionOccured);
+  }
+
+  @Test
+  public void testVerifyAggregationsMultiLevelMissingVal() throws Exception
+  {
+    List<AggregatorFactory> aggFactories = Arrays.<AggregatorFactory>asList(
+        new CountAggregatorFactory("count"),
+        new DoubleSumAggregatorFactory("idx", "index"),
+        new DoubleSumAggregatorFactory("rev", "revenue")
+    );
+
+    List<PostAggregator> postAggs = Arrays.<PostAggregator>asList(
+        new ArithmeticPostAggregator(
+            "divideStuff",
+            "/",
+            Arrays.<PostAggregator>asList(
+                new ArithmeticPostAggregator(
+                    "addStuff",
+                    "+",
+                    Arrays.asList(
+                        new FieldAccessPostAggregator("idx", "idx"),
+                        new ConstantPostAggregator("const", 1)
+                    )
+                ),
+                new ArithmeticPostAggregator(
+                    "subtractStuff",
+                    "-",
+                    Arrays.asList(
+                        new FieldAccessPostAggregator("rev", "rev2"),
+                        new ConstantPostAggregator("const", 1)
+                    )
+                )
+            )
+        ),
+        new ArithmeticPostAggregator(
+            "addStuff",
+            "+",
+            Arrays.<PostAggregator>asList(
+                new FieldAccessPostAggregator("divideStuff", "divideStuff"),
+                new FieldAccessPostAggregator("count", "count")
+            )
+        )
+    );
+
+    boolean exceptionOccured = false;
+
+    try {
+      Queries.verifyAggregations(aggFactories, postAggs);
+    }
+    catch (IllegalArgumentException e) {
+      exceptionOccured = true;
+    }
+
+    Assert.assertTrue(exceptionOccured);
+  }
+}

common/src/main/java/com/metamx/druid/aggregation/post/ArithmeticPostAggregator.java

@@ -68,6 +68,17 @@ public class ArithmeticPostAggregator implements PostAggregator
     }
   }
 
+  @Override
+  public boolean verifyFields(Set<String> fieldNames)
+  {
+    for (PostAggregator field : fields) {
+      if (!field.verifyFields(fieldNames)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   @Override
   public Comparator getComparator()
   {

common/src/main/java/com/metamx/druid/aggregation/post/ConstantPostAggregator.java

@@ -24,6 +24,7 @@ import org.codehaus.jackson.annotate.JsonProperty;
 
 import java.util.Comparator;
 import java.util.Map;
+import java.util.Set;
 
 /**
  */
@@ -42,6 +43,12 @@ public class ConstantPostAggregator implements PostAggregator
     this.constantValue = constantValue;
   }
 
+  @Override
+  public boolean verifyFields(Set<String> fields)
+  {
+    return true;
+  }
+
   @Override
   public Comparator getComparator()
   {

common/src/main/java/com/metamx/druid/aggregation/post/FieldAccessPostAggregator.java

@@ -19,11 +19,13 @@
 
 package com.metamx.druid.aggregation.post;
 
+import com.metamx.common.ISE;
 import org.codehaus.jackson.annotate.JsonCreator;
 import org.codehaus.jackson.annotate.JsonProperty;
 
 import java.util.Comparator;
 import java.util.Map;
+import java.util.Set;
 
 /**
  */
@@ -42,6 +44,12 @@ public class FieldAccessPostAggregator implements PostAggregator
     this.fieldName = fieldName;
   }
 
+  @Override
+  public boolean verifyFields(Set<String> fieldNames)
+  {
+    return fieldNames.contains(fieldName);
+  }
+
   @Override
   public Comparator getComparator()
   {

common/src/main/java/com/metamx/druid/aggregation/post/PostAggregator.java

@@ -24,6 +24,7 @@ import org.codehaus.jackson.annotate.JsonTypeInfo;
 
 import java.util.Comparator;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Functionally similar to an Aggregator. See the Aggregator interface for more comments.
@@ -36,6 +37,8 @@ import java.util.Map;
 })
 public interface PostAggregator
 {
+  public boolean verifyFields(Set<String> fieldNames);
+
   public Comparator getComparator();
 
   public Object compute(Map<String, Object> combinedAggregators);