Missing theta sketch unwrapper in metric maker #128

michael-mclawhorn · 2016-12-20T21:46:05Z

Fixed missing coercion theta sketch path.
Added a ton of tests.

cdeszaq · 2016-12-21T22:41:00Z

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

     *
-     * @return A post aggregation referencing a sketch value
+     * @deprecated use the static version {@link #getSketchField(MetricField)} by preference
     */
    protected PostAggregation getSketchField(String fieldName) {


Needs @Deprecated annotation

cdeszaq · 2016-12-22T19:45:23Z

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

+            return ((ThetaSketchEstimatePostAggregation) field).getField();
+        }
+
+        if (field instanceof ThetaSketchEstimatePostAggregation) {


Duplicate with the one above it?

cdeszaq · 2016-12-22T19:47:48Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

 import spock.lang.Specification
-
+import spock.lang.Unroll
 /**


Blank line needed

cdeszaq · 2016-12-22T20:37:28Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

-
+    @Unroll
+    def "getSketchField throws error when given a non sketch based field #fromMetric.name "() {
+        setup:


Empty setup block isn't useful

cdeszaq · 2016-12-22T20:38:38Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

-
+    @Unroll
+    def "getSketchField returns a sketch aggregation of type #expected.type for metric named: #fromMetric.name "() {
+        setup:


Empty setup block isn't doing anything

cdeszaq · 2016-12-22T20:44:13Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

@@ -115,64 +184,59 @@ class MetricMakerSpec extends Specification {
       then:
       thrown(IllegalArgumentException)
   }
+    def "When a metric has too many dependent metrics, an exception is thrown"(){
+        given: "One more than the number of dependent metrics this maker depends on."
+        List<String> dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()+1)


This arithmetic should be more spaced

cdeszaq · 2016-12-22T20:44:16Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy


        then:
        thrown(IllegalArgumentException)
    }

   def "When a claimed dependent metric does not exist in the maker's dictionary, an exception is thrown"(){
       given: "A list of dependent metrics, one of which doesn't exist"
-       List<String> dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()-1)
-       maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames))
+       List<String> dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS-1)


This arithmetic should be more spaced

cdeszaq · 2016-12-22T20:44:20Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

    def "When a metric has too few dependent metrics, an exception is thrown."(){
        given: "One fewer dependent metrics than this metric depends on"
-        List<String> dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()-1)
-        maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames))
+        List<String> dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS-1)


This arithmetic should be more spaced

cdeszaq · 2016-12-22T20:45:03Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy

+    }
+
+    def setup(){
+    }


Delete empty method

michael-mclawhorn · 2017-01-03T17:34:01Z

Responded to change requests

garyluoex

Just some personal preference things, feel free to leave it as is. 👍 (Just an addition question, is there a strong reason to make those functions static? It seems like it would add some work to get the field instead of reusing the code in the function which now takes a field instead of a string)

garyluoex · 2017-01-17T21:54:11Z

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

+     */
+    protected static PostAggregation getNumericField(MetricField field) {
+        // If the field is a sketch, wrap it in a sketch estimate, if it's an aggregation, create a post aggregation
+        // Otherwise it is a number post aggregation already


Is this comment necessary? It should be in the function java doc already right?

I agree that it is redundant. I'll consider removing it. I think it got duplicated up into the javadoc and wasn't pruned.

garyluoex · 2017-01-17T22:01:16Z

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

+        }
+
+        if (field instanceof ThetaSketchEstimatePostAggregation) {
+            return ((ThetaSketchEstimatePostAggregation) field).getField();
        }

        // Check for sketches, since we require them after this point
        if (!field.isSketch()) {


Should we move this if check above the previous two checks so that the code feels more logical?

Not exactly. There are three possibilities. The thing IS a sketch, it is an estimating wrapper AROUND a sketch, or otherwise. The way to test whether it's a simple wrapper around a sketch is whether it's one of the Estimator aggregations.

garyluoex · 2017-01-17T22:05:03Z

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

+     *
+     * @return A post aggregator representing a number field value
+     */
+    protected static PostAggregation getSketchField(MetricField field) {
        // SketchEstimatePostAggregations are just wrappers for the actual PostAggregation we want to return
        if (field instanceof SketchEstimatePostAggregation) {


It seems like SketchEstimatePostAggregation is deprecated, should we log something here to take note of it?

The nature of the situation is that the user has one of two sketch libraries implemented in their code. If they have the old sketch library in their code, then it's not a log-worthy event that this code is being used, because it's mandatory.

We'll probably pull the deprecated versions slightly after the next version bump.

garyluoex · 2017-01-17T22:17:55Z

...e/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy


        when:
-        maker.make(METRIC_NAME, dependentMetricNames)
+        getMakerInstance().make(METRIC_NAME, dependentMetricNames)

        then:
        thrown(IllegalArgumentException)


I personally prefer adding the error message to the exception check, just to make sure that it is the correct exception thrown.

fair enough

Tightened testing on metric maker errors Much more comprehensive testing of field coercions. Deprecated dictionary dependent field coercion instance methods in favor of static field->field coercion methods.

michael-mclawhorn force-pushed the MissingThetaSketchUnwrapperInMetricMaker branch from 3371ade to 1a5132a Compare December 20, 2016 21:51

michael-mclawhorn requested a review from cdeszaq December 20, 2016 21:55

michael-mclawhorn force-pushed the MissingThetaSketchUnwrapperInMetricMaker branch from 1a5132a to 92c40b8 Compare December 20, 2016 23:09

cdeszaq requested changes Dec 22, 2016

View reviewed changes

michael-mclawhorn force-pushed the MissingThetaSketchUnwrapperInMetricMaker branch from 92c40b8 to 8e2f1de Compare January 3, 2017 17:32

cdeszaq approved these changes Jan 5, 2017

View reviewed changes

cdeszaq added the NEED 1 REVIEW label Jan 10, 2017

garyluoex approved these changes Jan 17, 2017

View reviewed changes

garyluoex added MERGEABLE and removed NEED 1 REVIEW labels Jan 17, 2017

Added getSketchField unwrapper for theta sketches.

5f5da6a

Tightened testing on metric maker errors Much more comprehensive testing of field coercions. Deprecated dictionary dependent field coercion instance methods in favor of static field->field coercion methods.

michael-mclawhorn force-pushed the MissingThetaSketchUnwrapperInMetricMaker branch from 615c883 to 5f5da6a Compare January 18, 2017 17:34

michael-mclawhorn merged commit 5f5da6a into master Jan 18, 2017

michael-mclawhorn deleted the MissingThetaSketchUnwrapperInMetricMaker branch January 18, 2017 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing theta sketch unwrapper in metric maker #128

Missing theta sketch unwrapper in metric maker #128

michael-mclawhorn commented Dec 20, 2016

cdeszaq Dec 21, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

cdeszaq Dec 22, 2016

michael-mclawhorn commented Jan 3, 2017

garyluoex left a comment

garyluoex Jan 17, 2017

michael-mclawhorn Jan 17, 2017

garyluoex Jan 17, 2017

michael-mclawhorn Jan 17, 2017

garyluoex Jan 17, 2017

michael-mclawhorn Jan 17, 2017

garyluoex Jan 17, 2017

michael-mclawhorn Jan 17, 2017

michael-mclawhorn Jan 18, 2017

Missing theta sketch unwrapper in metric maker #128

Missing theta sketch unwrapper in metric maker #128

Conversation

michael-mclawhorn commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn commented Jan 3, 2017

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment