Fix dimension serialization problem dealing with nested queries in da…

…tasource, added tests for corresponding serialization

CHANGELOG.md

@@ -19,6 +19,10 @@ Current
 
 #### Changed:
 
+-  [Fix Dimension Serialization Problem with Nested Queries](https://github.com/yahoo/fili/pull/15)
+    * Modified `DimensionToDefaultDimensionSpec` serializer to serialize dimension to apiName if it is not the inner most query
+    * Added helper `hasInnerQuery` to `Util` in serializer package to determine if current query is the inner most query or not
+    * Added tests for `DimensionToDefaultDimensionSpec`
 
 ### Fixed:
 

fili-core/src/main/java/com/yahoo/bard/webservice/druid/serializers/DimensionToDefaultDimensionSpec.java

@@ -31,7 +31,8 @@ public void serialize(Dimension value, JsonGenerator gen, SerializerProvider pro
                 }
         );
 
-        if (physicalName.equals(apiName)) {
+        // serialize to only apiName if api and physical name is same or there are nested queries
+        if (physicalName.equals(apiName) || Util.hasInnerQuery(gen)) {
             gen.writeString(apiName);
         } else {
             gen.writeObject(new DefaultDimensionSpec(physicalName, apiName));

fili-core/src/main/java/com/yahoo/bard/webservice/druid/serializers/Util.java

@@ -9,6 +9,7 @@
 import com.fasterxml.jackson.core.JsonStreamContext;
 
 import java.util.Optional;
+import java.util.function.Function;
 
 /**
  * Utility functions for druid serializers.
@@ -24,24 +25,48 @@ public class Util {
      * @return  an Optional String of physical name
      */
     public static Optional<String> findPhysicalName(Dimension value, JsonGenerator gen) {
-        JsonStreamContext context = gen.getOutputContext();
         String apiName = value.getApiName();
         // Search for physical name
+        return mapNearestDruidQuery(
+                gen,
+                druidQuery -> druidQuery.getDataSource()
+                        .getPhysicalTables()
+                        .iterator()
+                        .next()
+                        .getPhysicalColumnName(apiName)
+        );
+    }
+
+    /**
+     * JSON tree walk to determine if there is a nested query below the current json node or not.
+     *
+     * @param gen  the Json Generator to retrieve the tree to walk on.
+     *
+     * @return  a Boolean where true indicates there are more nested query below this node, false otherwise
+     */
+    public static Boolean hasInnerQuery(JsonGenerator gen) {
+        return mapNearestDruidQuery(gen, druidQuery -> druidQuery.getInnerQuery() != null).orElse(false);
+    }
+
+    /**
+     * JSON tree walk to find the druid query context of the current context and apply handler to the DruidQuery,
+     * finds the current context if current context is a druid query.
+     *
+     * @param gen  the Json Generator to retrieve the tree to walk on.
+     * @param mapper  a function that takes an DruidQuery as an argument and return the final desired returned result.
+     *
+     * @return an Optional of the desired result T if DruidQuery is found, Optional.empty otherwise
+     */
+    public static <T> Optional<T> mapNearestDruidQuery(JsonGenerator gen, Function<DruidQuery, T> mapper) {
+        JsonStreamContext context = gen.getOutputContext();
+
         while (context != null) {
             Object parent = context.getCurrentValue();
             if (parent instanceof DruidQuery) {
-                return Optional.of(
-                        ((DruidQuery) parent)
-                                .getDataSource()
-                                .getPhysicalTables()
-                                .iterator()
-                                .next()
-                                .getPhysicalColumnName(apiName)
-                );
+                return Optional.of(mapper.apply((DruidQuery) parent));
             }
             context = context.getParent();
         }
-        // If we cannot find the physical name, then return empty optional.
         return Optional.empty();
     }
 }

fili-core/src/test/groovy/com/yahoo/bard/webservice/druid/serializers/DimensionToDefaultDimensionSpecSpec.groovy

@@ -0,0 +1,84 @@
+// Copyright 2016 Yahoo Inc.
+// Licensed under the terms of the Apache license. Please see LICENSE file distributed with this work for terms.
+package com.yahoo.bard.webservice.druid.serializers
+
+import static com.yahoo.bard.webservice.data.time.DefaultTimeGrain.HOUR
+import static org.joda.time.DateTimeZone.UTC
+
+import com.yahoo.bard.webservice.data.DruidQueryBuilder
+import com.yahoo.bard.webservice.data.PartialDataHandler
+import com.yahoo.bard.webservice.data.QueryBuildingTestingResources
+import com.yahoo.bard.webservice.data.dimension.Dimension
+import com.yahoo.bard.webservice.data.metric.LogicalMetric
+import com.yahoo.bard.webservice.data.metric.mappers.NoOpResultSetMapper
+import com.yahoo.bard.webservice.data.volatility.DefaultingVolatileIntervalsService
+import com.yahoo.bard.webservice.druid.model.query.DruidAggregationQuery
+import com.yahoo.bard.webservice.table.resolver.DefaultPhysicalTableResolver
+import com.yahoo.bard.webservice.web.DataApiRequest
+
+import com.fasterxml.jackson.databind.ObjectMapper
+
+import org.joda.time.DateTime
+import org.joda.time.Hours
+import org.joda.time.Interval
+
+import spock.lang.Specification
+
+/**
+ * Testing dimension serialization to default dimension spec.
+ */
+class DimensionToDefaultDimensionSpecSpec extends Specification {
+
+    ObjectMapper objectMapper
+    QueryBuildingTestingResources resources
+    DruidQueryBuilder builder
+    DataApiRequest apiRequest
+    DruidAggregationQuery<?> druidQuery
+
+    def setup() {
+        objectMapper = new ObjectMapper()
+        resources = new QueryBuildingTestingResources()
+        DefaultPhysicalTableResolver resolver = new DefaultPhysicalTableResolver(new PartialDataHandler(), new DefaultingVolatileIntervalsService())
+        builder = new DruidQueryBuilder(resources.logicalDictionary, resolver)
+        apiRequest = Mock(DataApiRequest)
+        LogicalMetric lm1 = new LogicalMetric(resources.simpleTemplateQuery, new NoOpResultSetMapper(), "lm1", null)
+
+        apiRequest.getTable() >> resources.lt12
+        apiRequest.getGranularity() >> HOUR.buildZonedTimeGrain(UTC)
+        apiRequest.getTimeZone() >> UTC
+        apiRequest.getLogicalMetrics() >> ([lm1])
+        apiRequest.getIntervals() >> [new Interval(new DateTime("2015"), Hours.ONE)]
+        apiRequest.getFilterDimensions() >> []
+        apiRequest.getTopN() >> OptionalInt.empty()
+        apiRequest.getSorts() >> ([])
+        apiRequest.getCount() >> OptionalInt.empty()
+    }
+
+    def "Serialize to apiName when apiName and physicalName of a dimension is the same"() {
+        given:
+        apiRequest.getDimensions() >> ([resources.d1])
+        druidQuery = builder.buildQuery(apiRequest, resources.simpleTemplateQuery)
+
+        expect:
+        objectMapper.writeValueAsString(druidQuery).contains('"dimensions":["dim1"]')
+    }
+
+    def "Serialize to dimension spec when the apiName and physicalName of a dimension is different"() {
+        given:
+        apiRequest.getDimensions() >> ([resources.d3])
+        druidQuery = builder.buildQuery(apiRequest, resources.simpleTemplateQuery)
+
+        expect:
+        objectMapper.writeValueAsString(druidQuery).contains('"dimensions":[{"type":"default","dimension":"age_bracket","outputName":"ageBracket"}]')
+    }
+
+    def "Serialize dimension of a nested query should only serialize the innermost query's dimension to a dimension spec"() {
+        given:
+        apiRequest.getDimensions() >> ([resources.d3])
+        druidQuery = builder.buildQuery(apiRequest, resources.simpleNestedTemplateQuery)
+        String serializedQuery = objectMapper.writeValueAsString(druidQuery)
+
+        expect:
+        serializedQuery.contains('"dimensions":[{"type":"default","dimension":"age_bracket","outputName":"ageBracket"}]') && serializedQuery.contains('"dimensions":["ageBracket"]')
+    }
+}