PR Feedback

CHANGELOG.md

@@ -163,7 +163,9 @@ Current
     * [JavaX Annotation API 1.2 -> 1.3](https://jcp.org/en/jsr/detail?id=250)
 
 ### Deprecated:
-- [Deprecated MetricMaker utility method in favor of using new field accesor on Metric](https://github.com/yahoo/fili/pull/124)
+- [Metric configuration deprecations](https://github.com/yahoo/fili/pull/124)
+    * Deprecated superfluous constructor of `FilteredAggregator` with superfluous argument 
+    * Deprecated MetricMaker utility method in favor of using new field accessor on Metric
 
 - [Deprecated MetricMaker.getDependentQuery lookup method in favor of simpler direct access](https://github.com/yahoo/fili/pull/124)
 

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMaker.java

@@ -32,24 +32,24 @@ public class ArithmeticMaker extends MetricMaker {
 
     private final ArithmeticPostAggregationFunction function;
 
-    private final Function<String, ResultSetMapper> resultSetMapperBuilder;
+    private final Function<String, ResultSetMapper> resultSetMapperSupplier;
 
     /**
      * Constructor.
      *
      * @param metricDictionary  The dictionary used to resolve dependent metrics when building the LogicalMetric
      * @param function  The arithmetic operation performed by the LogicalMetrics constructed by this maker
-     * @param resultSetMapperBuilder  A builder for a function to be applied to the result that is returned by the query
-    that is built from the LogicalMetric which is built by this maker.
+     * @param resultSetMapperSupplier  A function that takes a metric column name and produces at build time, a result
+     * set mapper.
      */
     protected ArithmeticMaker(
             MetricDictionary metricDictionary,
             ArithmeticPostAggregationFunction function,
-            Function<String, ResultSetMapper> resultSetMapperBuilder
+            Function<String, ResultSetMapper> resultSetMapperSupplier
     ) {
         super(metricDictionary);
         this.function = function;
-        this.resultSetMapperBuilder = resultSetMapperBuilder;
+        this.resultSetMapperSupplier = resultSetMapperSupplier;
     }
 
     /**
@@ -86,7 +86,7 @@ public ArithmeticMaker(MetricDictionary metricDictionary, ArithmeticPostAggregat
         this(
                 metricDictionary,
                 function,
-                (Function<String, ResultSetMapper>) (String name) -> new SketchRoundUpMapper(name)
+                (Function<String, ResultSetMapper>) SketchRoundUpMapper::new
         );
     }
 
@@ -105,10 +105,7 @@ protected LogicalMetric makeInner(String metricName, List<String> dependentMetri
         ));
 
         TemplateDruidQuery query = getMergedQuery(dependentMetrics).withPostAggregations(postAggregations);
-
-        // Note: We need to pass everything through ColumnMapper
-        // We need to refactor this to be a list.
-        return new LogicalMetric(query, resultSetMapperBuilder.apply(metricName), metricName);
+        return new LogicalMetric(query, resultSetMapperSupplier.apply(metricName), metricName);
     }
 
     @Override

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/ConstantMaker.java

@@ -8,6 +8,9 @@
 import com.yahoo.bard.webservice.druid.model.postaggregation.ConstantPostAggregation;
 import com.yahoo.bard.webservice.druid.model.postaggregation.PostAggregation;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -18,6 +21,7 @@
 public class ConstantMaker extends MetricMaker {
 
     private static final int DEPENDENT_METRICS_REQUIRED = 1;
+    protected static final Logger LOG = LoggerFactory.getLogger(ConstantMaker.class);
 
     /**
      * Constructor.

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/FilteredAggregationMaker.java

@@ -13,6 +13,9 @@
 
 import com.google.common.collect.ImmutableSet;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.Collections;
 import java.util.List;
 
@@ -23,6 +26,8 @@
  */
 public class FilteredAggregationMaker extends MetricMaker {
 
+    protected static final Logger LOG = LoggerFactory.getLogger(ConstantMaker.class);
+
     /**
      * The filter being applied to the metrics created.
      */

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java

@@ -24,9 +24,10 @@
  */
 public abstract class MetricMaker {
 
-    protected static final Logger LOG = LoggerFactory.getLogger(MetricMaker.class);
+    private static final Logger LOG = LoggerFactory.getLogger(MetricMaker.class);
 
     public static final NoOpResultSetMapper NO_OP_MAPPER = new NoOpResultSetMapper();
+
     /**
      * The metric dictionary from which dependent metrics will be resolved.
      */

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/RawAggregationMetricMaker.java

@@ -27,7 +27,7 @@ public abstract class RawAggregationMetricMaker extends MetricMaker {
      *
      * @param metrics  A mapping of metric names to the corresponding LogicalMetrics. Used to resolve metric names
      * when making the logical metric.
-     * @param aggregationFactory  A method to produce an aggregation from a name and field name
+     * @param aggregationFactory  Produce an aggregation from a name and field name
      */
     public RawAggregationMetricMaker(
             MetricDictionary metrics,
@@ -69,7 +69,7 @@ protected int getDependentMetricsRequired() {
      *
      * @return The result set mapper bound to a metric being produced by this maker.
      */
-    public ResultSetMapper getResultSetMapper(String metricName) {
+    protected ResultSetMapper getResultSetMapper(String metricName) {
         return NO_OP_MAPPER;
     }
 }

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/ThetaSketchSetOperationMaker.java

@@ -54,14 +54,7 @@ protected LogicalMetric makeInner(String metricName, List<String> dependentMetri
         );
 
         PostAggregation estimate = new ThetaSketchEstimatePostAggregation(metricName, setPostAggregation);
-
-        TemplateDruidQuery query = new TemplateDruidQuery(
-                mergedQuery.getAggregations(),
-                Collections.singleton(estimate),
-                mergedQuery.getInnerQuery(),
-                mergedQuery.getTimeGrain()
-        );
-
+        TemplateDruidQuery query = mergedQuery.withPostAggregations(Collections.singleton(estimate));
         return new LogicalMetric(query, new SketchRoundUpMapper(metricName), metricName);
     }
 

fili-core/src/main/java/com/yahoo/bard/webservice/data/metric/mappers/ColumnMapper.java

@@ -4,7 +4,10 @@
 
 /**
  * A ColumnMapper is a ResultSetMapper that operates on a single column.
+ *
+ * @deprecated with-like functionality no longer needed because delayed construction is being used instead
  */
+@Deprecated
 public interface ColumnMapper {
 
     /**

fili-core/src/main/java/com/yahoo/bard/webservice/data/metric/mappers/NoOpResultSetMapper.java

@@ -26,6 +26,7 @@ protected Schema map(Schema schema) {
     }
 
     @Override
+    @Deprecated
     public ResultSetMapper withColumnName(String newColumnName) {
         return this;
     }

fili-core/src/main/java/com/yahoo/bard/webservice/data/metric/mappers/SketchRoundUpMapper.java

@@ -69,6 +69,7 @@ protected Schema map(Schema schema) {
     }
 
     @Override
+    @Deprecated
     public ResultSetMapper withColumnName(String newColumnName) {
         return new SketchRoundUpMapper(newColumnName);
     }

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/aggregation/FilteredAggregation.java

@@ -40,12 +40,27 @@ public FilteredAggregation(@NotNull String name, Aggregation aggregation, Filter
     }
 
     /**
-     * Splits an Aggregation for 2-pass aggregation into an inner filtered aggregation & outer aggregation. The
-     * outer aggregation is obtained by unwrapping the inner filtered aggregation and getting just the aggregation.
-     * The outer aggregation fieldName will reference the inner aggregation name. The inner aggregation is unmodified.
+     * Constructor.
      *
-     * @return A pair where pair.left is the outer aggregation and pair.right is the inner.
+     * @param name  Name of the filtered aggregator
+     * @param fieldName  Field name to be considered to apply the metric filter
+     * @param aggregation  Existing aggregator being filtered
+     * @param filter  filter to apply to that aggregator
+     *
+     * @deprecated Filtered Aggregations do not have their own field name, they use the one from their aggregator
      */
+    @Deprecated
+    public FilteredAggregation(@NotNull String name, String fieldName, Aggregation aggregation, Filter filter) {
+        this(name, aggregation, filter);
+    }
+
+        /**
+         * Splits an Aggregation for 2-pass aggregation into an inner filtered aggregation & outer aggregation. The
+         * outer aggregation is obtained by unwrapping the inner filtered aggregation and getting just the aggregation.
+         * The outer aggregation fieldName will reference the inner aggregation name. The inner aggregation is unmodified.
+         *
+         * @return A pair where pair.left is the outer aggregation and pair.right is the inner.
+         */
     @Override
     public Pair<Aggregation, Aggregation> nest() {
         String nestingName = this.getName();

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/ArithmeticMakerSpec.groovy

@@ -23,6 +23,8 @@ import spock.lang.Shared
 import spock.lang.Specification
 import spock.lang.Unroll
 
+import java.util.function.Function
+
 class ArithmeticMakerSpec extends Specification {
     LogicalMetric dayAvgPageViewsPerTotalPageViews
     LogicalMetric dayAvgPageViewsPerTotalPageViewsRoundedUp
@@ -43,7 +45,7 @@ class ArithmeticMakerSpec extends Specification {
         ArithmeticMaker divisionMaker = new ArithmeticMaker(
                 metricDictionary,
                 ArithmeticPostAggregation.ArithmeticPostAggregationFunction.DIVIDE,
-                new NoOpResultSetMapper()
+                (Function) { String it -> return new NoOpResultSetMapper() }
         )
         ArithmeticMaker roundUpDivisionMaker = new ArithmeticMaker(
                 metricDictionary,

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/ConstantMakerSpec.groovy

@@ -36,7 +36,7 @@ class ConstantMakerSpec extends Specification {
         maker.make(AGGREGATION_NAME, Double.toString(CONSTANT_VALUE)) == expectedMetric
     }
 
-    def """A NumberFormatException is thrown if the dependent metric passed to ConstantMaker is not a number."""(){
+    def "A NumberFormatException is thrown if the dependent metric passed to ConstantMaker is not a number."(){
         given:
         String invalidValue = "I'm not a number."
 

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/FilteredAggregationMakerSpec.groovy

@@ -4,7 +4,6 @@
 package com.yahoo.bard.webservice.data.config.metric.makers
 
 import com.yahoo.bard.webservice.data.dimension.Dimension
-import com.yahoo.bard.webservice.data.dimension.DimensionField
 import com.yahoo.bard.webservice.data.dimension.KeyValueStore
 import com.yahoo.bard.webservice.data.dimension.SearchProvider
 import com.yahoo.bard.webservice.data.dimension.impl.KeyValueStoreDimension
@@ -32,7 +31,7 @@ public class FilteredAggregationMakerSpec extends Specification{
         given: "The name of the metric the maker depends on, and the maker itself"
 
         LogicalMetric metric = longSumMaker.make("longSum", DEPENDENT_METRIC_NAME)
-        Dimension dim = new KeyValueStoreDimension("d", "des", new LinkedHashSet<DimensionField>(), Mock(KeyValueStore), Mock(SearchProvider))
+        Dimension dim = new KeyValueStoreDimension("d", "des", [] as Set, Mock(KeyValueStore), Mock(SearchProvider))
         Filter filter = new SelectorFilter(dim, "1")
 
         MetricMaker maker = new FilteredAggregationMaker(metricDictionary, filter)

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/RawAggregationMetricMakerImplsSpec.groovy

@@ -1,3 +1,5 @@
+// Copyright 2016 Yahoo Inc.
+// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
 package com.yahoo.bard.webservice.data.config.metric.makers
 
 import com.yahoo.bard.webservice.data.metric.LogicalMetric
@@ -18,18 +20,18 @@ import spock.lang.Specification
 import spock.lang.Unroll
 
 /**
- *
- * Created by mclawhor on 12/19/16.
+ * Tests for raw aggregation makers.
  */
 class RawAggregationMetricMakerImplsSpec extends Specification {
 
     public static String NAME = "FOO"
     public static String FIELD_NAME = "BAR"
 
     @Unroll
-    def "Expected numeric aggregation is produced for #makerClass.getSimpleName()"() {
+    def "Expected numeric aggregation is produced for #makerClass.simpleName"() {
         setup:
         RawAggregationMetricMaker maker = makerClass.newInstance()
+
         expect:
         maker.make(NAME, FIELD_NAME) == makeNumericMetric(aggregation)
 
@@ -44,9 +46,10 @@ class RawAggregationMetricMakerImplsSpec extends Specification {
     }
 
     @Unroll
-    def "Expected sketch aggregation is produced for #makerClass.getSimpleName()"() {
+    def "Expected sketch aggregation is produced for #makerClass.simpleName"() {
         setup:
         RawAggregationMetricMaker maker = makerClass.newInstance((MetricDictionary) null, 5)
+
         expect:
         maker.make(NAME, FIELD_NAME) == makeSketchMetric(aggregation)
 
@@ -57,7 +60,6 @@ class RawAggregationMetricMakerImplsSpec extends Specification {
 
     }
 
-
     /*
        It feels like cheating to duplicate so much of the makeInner from the class under test, but Mocking a logical
        metric can't be more accurate than this and the test primarily tests the subclasses integrating correctly.