review comments

remove toShortString minor other changes
chinnurtb · May 6, 2014 · 66450ca · 66450ca
1 parent f272599
commit 66450ca
Show file tree

Hide file tree

Showing 16 changed files with 71 additions and 55 deletions.
diff --git a/processing/src/main/java/io/druid/query/DataSource.java b/processing/src/main/java/io/druid/query/DataSource.java
@@ -37,7 +37,5 @@
               })
 public interface DataSource
 {
-  public Iterable<String> getNames();
-
-  public String toShortString();
+  public List<String> getNames();
 }
diff --git a/processing/src/main/java/io/druid/query/DataSourceUtil.java b/processing/src/main/java/io/druid/query/DataSourceUtil.java
@@ -0,0 +1,31 @@
+/*
+ * Druid - a distributed column store.
+ * Copyright (C) 2012, 2013  Metamarkets Group Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+package io.druid.query;
+
+import java.util.List;
+
+public class DataSourceUtil
+{
+  public static String getMetricName(DataSource dataSource)
+  {
+    final List<String> names = dataSource.getNames();
+    return names.size() == 1 ? names.get(0) : names.toString();
+  }
+}
diff --git a/processing/src/main/java/io/druid/query/QueryDataSource.java b/processing/src/main/java/io/druid/query/QueryDataSource.java
@@ -25,6 +25,8 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonTypeName;
 
+import java.util.List;
+
 @JsonTypeName("query")
 public class QueryDataSource implements DataSource
 {
@@ -38,17 +40,11 @@ public QueryDataSource(@JsonProperty("query") Query query)
   }
 
   @Override
-  public Iterable<String> getNames()
+  public List<String> getNames()
   {
     return query.getDataSource().getNames();
   }
 
-  @Override
-  public String toShortString()
-  {
-    return query.getDataSource().toShortString();
-  }
-
   @JsonProperty
   public Query getQuery()
   {

diff --git a/processing/src/main/java/io/druid/query/TableDataSource.java b/processing/src/main/java/io/druid/query/TableDataSource.java
@@ -25,6 +25,7 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.google.common.collect.Lists;
 
+import java.util.Arrays;
 import java.util.List;
 
 @JsonTypeName("table")
@@ -45,15 +46,9 @@ public String getName(){
   }
 
   @Override
-  public Iterable<String> getNames()
+  public List<String> getNames()
   {
-    return Lists.newArrayList(name);
-  }
-
-  @Override
-  public String toShortString()
-  {
-    return name;
+    return Arrays.asList(name);
   }
 
   public String toString() { return name; }

diff --git a/processing/src/main/java/io/druid/query/UnionDataSource.java b/processing/src/main/java/io/druid/query/UnionDataSource.java
@@ -27,10 +27,9 @@
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
 
 import java.util.List;
-import java.util.SortedSet;
-import java.util.TreeSet;
 
 public class UnionDataSource implements DataSource
 {
@@ -45,30 +44,24 @@ public UnionDataSource(@JsonProperty("dataSources") List<TableDataSource> dataSo
   }
 
   @Override
-  public Iterable<String> getNames()
+  public List<String> getNames()
   {
-   return Iterables.concat(Iterables.transform(dataSources, new Function<DataSource, Iterable<String>>()
-   {
-     @Override
-     public Iterable<String> apply(DataSource input)
-     {
-       return input.getNames();
-     }
-   }));
-  }
-
-  @Override
-  public String toShortString()
-  {
-    SortedSet<String> str = new TreeSet<>();
-    for(DataSource ds : dataSources){
-      str.add(ds.toShortString());
-    }
-    return str.toString();
+    return Lists.transform(
+        dataSources,
+        new Function<TableDataSource, String>()
+        {
+          @Override
+          public String apply(TableDataSource input)
+          {
+            return Iterables.getOnlyElement(input.getNames());
+          }
+        }
+    );
   }
 
   @JsonProperty
-  public List<TableDataSource> getDataSources(){
+  public List<TableDataSource> getDataSources()
+  {
     return dataSources;
   }
 

diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java
@@ -35,6 +35,7 @@
 import io.druid.data.input.MapBasedRow;
 import io.druid.data.input.Row;
 import io.druid.query.DataSource;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.IntervalChunkingQueryRunner;
 import io.druid.query.Query;
 import io.druid.query.QueryDataSource;
@@ -163,7 +164,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(GroupByQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser3(String.format("%,d dims", query.getDimensions().size()))
         .setUser4("groupBy")
         .setUser5(Joiner.on(",").join(query.getIntervals()))

diff --git a/processing/src/main/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java
@@ -35,6 +35,7 @@
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.common.utils.JodaUtils;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.Query;
 import io.druid.query.QueryRunner;
 import io.druid.query.QueryToolChest;
@@ -147,7 +148,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(SegmentMetadataQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4(query.getType())
         .setUser5(Joiner.on(",").join(query.getIntervals()))
         .setUser6(String.valueOf(query.hasFilters()))

diff --git a/processing/src/main/java/io/druid/query/search/SearchQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/search/SearchQueryQueryToolChest.java
@@ -38,6 +38,7 @@
 import com.metamx.emitter.service.ServiceMetricEvent;
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.IntervalChunkingQueryRunner;
 import io.druid.query.Query;
 import io.druid.query.QueryRunner;
@@ -121,7 +122,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(SearchQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4("search")
         .setUser5(COMMA_JOIN.join(query.getIntervals()))
         .setUser6(String.valueOf(query.hasFilters()))

diff --git a/processing/src/main/java/io/druid/query/select/SelectQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/select/SelectQueryQueryToolChest.java
@@ -34,6 +34,7 @@
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.granularity.QueryGranularity;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.IntervalChunkingQueryRunner;
 import io.druid.query.Query;
 import io.druid.query.QueryConfig;
@@ -123,7 +124,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(SelectQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4("Select")
         .setUser5(COMMA_JOIN.join(query.getIntervals()))
         .setUser6(String.valueOf(query.hasFilters()))

diff --git a/processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQueryQueryToolChest.java
@@ -33,6 +33,7 @@
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.query.BySegmentSkippingQueryRunner;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.Query;
 import io.druid.query.QueryRunner;
 import io.druid.query.QueryToolChest;
@@ -117,7 +118,7 @@ public Sequence<Result<TimeBoundaryResultValue>> mergeSequences(Sequence<Sequenc
   public ServiceMetricEvent.Builder makeMetricBuilder(TimeBoundaryQuery query)
   {
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4(query.getType())
         .setUser6("false");
   }

diff --git a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java
@@ -33,6 +33,7 @@
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.granularity.QueryGranularity;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.IntervalChunkingQueryRunner;
 import io.druid.query.Query;
 import io.druid.query.QueryCacheHelper;
@@ -122,7 +123,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(TimeseriesQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4("timeseries")
         .setUser5(COMMA_JOIN.join(query.getIntervals()))
         .setUser6(String.valueOf(query.hasFilters()))

diff --git a/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java
@@ -37,6 +37,7 @@
 import io.druid.collections.OrderedMergeSequence;
 import io.druid.granularity.QueryGranularity;
 import io.druid.query.CacheStrategy;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.IntervalChunkingQueryRunner;
 import io.druid.query.Query;
 import io.druid.query.QueryCacheHelper;
@@ -138,7 +139,7 @@ public ServiceMetricEvent.Builder makeMetricBuilder(TopNQuery query)
     }
 
     return new ServiceMetricEvent.Builder()
-        .setUser2(query.getDataSource().toShortString())
+        .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
         .setUser4(String.format("topN/%s/%s", query.getThreshold(), query.getDimensionSpec().getDimension()))
         .setUser5(COMMA_JOIN.join(query.getIntervals()))
         .setUser6(String.valueOf(query.hasFilters()))

diff --git a/processing/src/test/java/io/druid/query/DataSourceTest.java b/processing/src/test/java/io/druid/query/DataSourceTest.java
@@ -101,7 +101,6 @@ public void testUnionDataSource() throws Exception
         Lists.newArrayList("ds1", "ds2"),
         Lists.newArrayList(dataSource.getNames())
     );
-    Assert.assertEquals(Lists.newArrayList("ds1", "ds2").toString(), dataSource.toShortString());
 
     final DataSource serde = jsonMapper.readValue(jsonMapper.writeValueAsString(dataSource), DataSource.class);
     Assert.assertEquals(dataSource, serde);

diff --git a/server/src/main/java/io/druid/server/AsyncQueryForwardingServlet.java b/server/src/main/java/io/druid/server/AsyncQueryForwardingServlet.java
@@ -31,6 +31,7 @@
 import io.druid.client.RoutingDruidClient;
 import io.druid.guice.annotations.Json;
 import io.druid.guice.annotations.Smile;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.Query;
 import io.druid.server.log.RequestLogger;
 import io.druid.server.router.QueryHostFinder;
@@ -282,7 +283,7 @@ public ClientResponse<OutputStream> done(ClientResponse<OutputStream> clientResp
 
           emitter.emit(
               new ServiceMetricEvent.Builder()
-                  .setUser2(theQuery.getDataSource().toShortString())
+                  .setUser2(DataSourceUtil.getMetricName(theQuery.getDataSource()))
                   .setUser4(theQuery.getType())
                   .setUser5(COMMA_JOIN.join(theQuery.getIntervals()))
                   .setUser6(String.valueOf(theQuery.hasFilters()))

diff --git a/server/src/main/java/io/druid/server/QueryResource.java b/server/src/main/java/io/druid/server/QueryResource.java
@@ -34,6 +34,7 @@
 import com.metamx.emitter.service.ServiceMetricEvent;
 import io.druid.guice.annotations.Json;
 import io.druid.guice.annotations.Smile;
+import io.druid.query.DataSourceUtil;
 import io.druid.query.Query;
 import io.druid.query.QuerySegmentWalker;
 import io.druid.server.log.RequestLogger;
@@ -131,7 +132,7 @@ public void doPost(
 
       emitter.emit(
           new ServiceMetricEvent.Builder()
-              .setUser2(query.getDataSource().toShortString())
+              .setUser2(DataSourceUtil.getMetricName(query.getDataSource()))
               .setUser4(query.getType())
               .setUser5(COMMA_JOIN.join(query.getIntervals()))
               .setUser6(String.valueOf(query.hasFilters()))

diff --git a/server/src/main/java/io/druid/server/coordination/ServerManager.java b/server/src/main/java/io/druid/server/coordination/ServerManager.java
@@ -322,11 +322,6 @@ public QueryRunner<T> apply(PartitionChunk<ReferenceCountingSegment> input)
 
   private String getDataSourceName(DataSource dataSource)
   {
-    Preconditions.checkArgument(
-        dataSource instanceof TableDataSource,
-        "Subqueries and Unions are only supported in the broker"
-    );
-
     return Iterables.getOnlyElement(dataSource.getNames());
   }