Fixed typo change in Changelog

Made NoOp default behavior for ArithmeticMaker Change MakerSpec tests and erased some of the names
yahoo · Dec 21, 2016 · f3896d8 · f3896d8
1 parent 1453c43
commit f3896d8
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -170,7 +170,7 @@ Current
 - [Deprecated MetricMaker.getDependentQuery lookup method in favor of simpler direct access](https://github.com/yahoo/fili/pull/124)
 
 - [Default DimensionColumn name to use apiName instead of physicalName](https://github.com/yahoo/fili/pull/115)
-    * Deprecated `TableUtils::getColumnNames(DataApiRequest, DruidAggregationQuery, PhysicalTable)` returning dimension physical ltername,
+    * Deprecated `TableUtils::getColumnNames(DataApiRequest, DruidAggregationQuery, PhysicalTable)` returning dimension physical name,
      in favor of `TableUtils::getColumnNames(DataApiRequest, DruidAggregationQuery)` returning dimension api name
     * Deprecated `DimensionColumn::DimensionColumn addNewDimensionColumn(Schema, Dimension, PhysicalTable)` in favor of
      `DimensionColumn::DimensionColumn addNewDimensionColumn(Schema, Dimension)` which uses api name instead of physical name as column identifier for columns

diff --git a/...re/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMaker.java b/...re/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMaker.java
@@ -7,7 +7,6 @@
 import com.yahoo.bard.webservice.data.metric.TemplateDruidQuery;
 import com.yahoo.bard.webservice.data.metric.mappers.ColumnMapper;
 import com.yahoo.bard.webservice.data.metric.mappers.ResultSetMapper;
-import com.yahoo.bard.webservice.data.metric.mappers.SketchRoundUpMapper;
 import com.yahoo.bard.webservice.druid.model.postaggregation.ArithmeticPostAggregation;
 import com.yahoo.bard.webservice.druid.model.postaggregation.ArithmeticPostAggregation.ArithmeticPostAggregationFunction;
 import com.yahoo.bard.webservice.druid.model.postaggregation.PostAggregation;
@@ -82,11 +81,10 @@ public ArithmeticMaker(
      * @param function  The arithmetic operation performed by the LogicalMetrics constructed by this maker
      */
     public ArithmeticMaker(MetricDictionary metricDictionary, ArithmeticPostAggregationFunction function) {
-        // TODO: Deprecate me, mappers should always be specified at creation time, not implicitly
         this(
                 metricDictionary,
                 function,
-                (Function<String, ResultSetMapper>) SketchRoundUpMapper::new
+                (Function<String, ResultSetMapper>) (ignore) -> NO_OP_MAPPER
         );
     }
 

diff --git a/...est/groovy/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMakerSpec.groovy b/...est/groovy/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMakerSpec.groovy
@@ -26,8 +26,13 @@ import spock.lang.Unroll
 import java.util.function.Function
 
 class ArithmeticMakerSpec extends Specification {
-    LogicalMetric dayAvgPageViewsPerTotalPageViews
-    LogicalMetric dayAvgPageViewsPerTotalPageViewsRoundedUp
+    public static final String AGG_AVERAGE_NAME = "aggregationMetric"
+    public static final String METRIC_FIELD_NAME = "aggregationField"
+    public static final String AVERAGE_PER_OTHER_METRIC_NAME = "averagePerOtherMetric"
+    public static
+    final String AVERAGE_PER_OTHER_METRIC_ROUNDED_METRIC_NAME = "averagePerOtherMetricRounded"
+    LogicalMetric unRoundedMetric
+    LogicalMetric roundedUpMetric
 
     private static final int SKETCH_SIZE = 16000
     //Neither of these relies on a metric dictionary.
@@ -44,41 +49,43 @@ class ArithmeticMakerSpec extends Specification {
 
         ArithmeticMaker divisionMaker = new ArithmeticMaker(
                 metricDictionary,
-                ArithmeticPostAggregation.ArithmeticPostAggregationFunction.DIVIDE,
-                (Function) { String it -> return new NoOpResultSetMapper() }
+                ArithmeticPostAggregation.ArithmeticPostAggregationFunction.DIVIDE
         )
         ArithmeticMaker roundUpDivisionMaker = new ArithmeticMaker(
                 metricDictionary,
                 ArithmeticPostAggregation.ArithmeticPostAggregationFunction.DIVIDE,
+                (Function) { String it -> return new SketchRoundUpMapper() }
         )
 
-        MetricInstance pageViews = new MetricInstance("PAGE_VIEWS", new LongSumMaker(metricDictionary), "PAGE_VIEWS")
+        MetricInstance pageViews = new MetricInstance(METRIC_FIELD_NAME, new LongSumMaker(metricDictionary),
+                METRIC_FIELD_NAME
+        )
         metricDictionary.add(pageViews.make())
 
-        MetricInstance dayAvgPageViews = new MetricInstance(
-                "DAY_AVG_PAGE_VIEWS",
+        MetricInstance aggregationAverageMetric = new MetricInstance(
+                AGG_AVERAGE_NAME,
                 new AggregationAverageMaker(metricDictionary, DAY),
-                "PAGE_VIEWS"
+                METRIC_FIELD_NAME
         )
-        metricDictionary.add(dayAvgPageViews.make())
+        metricDictionary.add(aggregationAverageMetric.make())
 
-        MetricInstance dayAvgPageViewsPerTotalPageViewsPM = new MetricInstance(
-                "DAY_AVG_PAGE_VIEWS_PER_TOTAL_PAGE_VIEWS",
+        MetricInstance averagePerOtherMetricMetric = new MetricInstance(
+                AVERAGE_PER_OTHER_METRIC_NAME,
                 divisionMaker,
-                "DAY_AVG_PAGE_VIEWS",
-                "PAGE_VIEWS"
+                AGG_AVERAGE_NAME,
+                METRIC_FIELD_NAME
         )
-        dayAvgPageViewsPerTotalPageViews = dayAvgPageViewsPerTotalPageViewsPM.make()
-        metricDictionary.add(dayAvgPageViewsPerTotalPageViews)
+        unRoundedMetric = averagePerOtherMetricMetric.make()
+        metricDictionary.add(unRoundedMetric)
 
         MetricInstance dayAvgPageViewsPerTotalPageViewsRoundedUpPM = new MetricInstance(
-                "DAY_AVG_PAGE_VIEWS_PER_TOTAL_PAGE_VIEWS_ROUNDED_UP",
+                AVERAGE_PER_OTHER_METRIC_ROUNDED_METRIC_NAME,
                 roundUpDivisionMaker,
-                "DAY_AVG_PAGE_VIEWS",
-                "PAGE_VIEWS"
+                AGG_AVERAGE_NAME,
+                METRIC_FIELD_NAME
         )
-        dayAvgPageViewsPerTotalPageViewsRoundedUp = dayAvgPageViewsPerTotalPageViewsRoundedUpPM.make()
-        metricDictionary.add(dayAvgPageViewsPerTotalPageViewsRoundedUp)
+        roundedUpMetric = dayAvgPageViewsPerTotalPageViewsRoundedUpPM.make()
+        metricDictionary.add(roundedUpMetric)
     }
 
     def cleanupSpec() {
@@ -125,7 +132,7 @@ class ArithmeticMakerSpec extends Specification {
         )
         LogicalMetric expectedMetric = new LogicalMetric(
             expectedQuery,
-            new SketchRoundUpMapper(metricName),
+            MetricMaker.NO_OP_MAPPER,
             metricName
         )
 
@@ -143,16 +150,17 @@ class ArithmeticMakerSpec extends Specification {
 
     def "ArithmeticMaker supports dependent metrics with nested query"() {
         expect:
-        dayAvgPageViewsPerTotalPageViews.getTemplateDruidQuery().nested
+        unRoundedMetric.getTemplateDruidQuery().nested
     }
 
     def "When a ResultMapper is explicitly passed, it creates the LogicalMetric using the correct mapper"() {
         expect:
-        dayAvgPageViewsPerTotalPageViews.calculation.class == NoOpResultSetMapper
+
+        unRoundedMetric.calculation.class == NoOpResultSetMapper
     }
 
     def "When a ResultMapper is not passed, it creates the LogicalMetric using the default SketchSetOperationMaker"() {
         expect:
-        dayAvgPageViewsPerTotalPageViewsRoundedUp.calculation.class == SketchRoundUpMapper
+        roundedUpMetric.calculation.class == SketchRoundUpMapper
     }
 }