diff --git a/CHANGELOG.md b/CHANGELOG.md index b292a635e2..e82005773d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Current ### Added: +- [Added comprehensive tests for `MetricMaker` field coercion methods](https://github.com/yahoo/fili/pull/128) + - [Added MetricField accessor to the interface of LogicalMetric](https://github.com/yahoo/fili/pull/124) * Previously accessing the metric field involved using three method calls @@ -157,6 +159,11 @@ Current * [JavaX Annotation API 1.2 -> 1.3](https://jcp.org/en/jsr/detail?id=250) ### Deprecated: + +- [Moved to static implementations for numeric and sketch coercion helper methods](https://github.com/yahoo/fili/pull/128) + * `MetricMaker.getSketchField(String fieldName)` rather use `MetricMaker.getSketchField(MetricField field)` + * `MetricMaker.getNumericField(String fieldName)` rather use `MetricMaker.getNumericField(MetricField field)` + - [Deprecated MetricMaker utility method in favor of using new field accesor on Metric](https://github.com/yahoo/fili/pull/124) - [Deprecated MetricMaker.getDependentQuery lookup method in favor of simpler direct access](https://github.com/yahoo/fili/pull/124) @@ -171,6 +178,8 @@ Current ### Fixed: +- [Added missing coverage for `ThetaSketchEstimate` unwrapping in `MetricMaker.getSketchField`](https://github.com/yahoo/fili/pull/128) + - [`DataSource::getNames` now returns Fili identifiers, not fact store identifiers](https://github.com/yahoo/fili/pull/125/files) - [Fix and refactor role based filter to allow CORS](https://github.com/yahoo/fili/pull/99) diff --git a/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy b/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy index 3948c77092..f8734cde95 100644 --- a/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy +++ b/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy @@ -2,23 +2,28 @@ // 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 static com.yahoo.bard.webservice.druid.model.postaggregation.ArithmeticPostAggregation.ArithmeticPostAggregationFunction.MULTIPLY +import static com.yahoo.bard.webservice.druid.model.postaggregation.SketchSetOperationPostAggFunction.UNION + import com.yahoo.bard.webservice.data.metric.LogicalMetric import com.yahoo.bard.webservice.data.metric.MetricDictionary import com.yahoo.bard.webservice.data.metric.TemplateDruidQuery import com.yahoo.bard.webservice.data.metric.mappers.NoOpResultSetMapper +import com.yahoo.bard.webservice.data.metric.mappers.SketchRoundUpMapper import com.yahoo.bard.webservice.druid.model.aggregation.Aggregation import com.yahoo.bard.webservice.druid.model.aggregation.LongSumAggregation -import com.yahoo.bard.webservice.druid.model.aggregation.SketchMergeAggregation +import com.yahoo.bard.webservice.druid.model.aggregation.ThetaSketchAggregation +import com.yahoo.bard.webservice.druid.model.postaggregation.ArithmeticPostAggregation import com.yahoo.bard.webservice.druid.model.postaggregation.ConstantPostAggregation import com.yahoo.bard.webservice.druid.model.postaggregation.FieldAccessorPostAggregation import com.yahoo.bard.webservice.druid.model.postaggregation.PostAggregation +import com.yahoo.bard.webservice.druid.model.postaggregation.ThetaSketchSetOperationPostAggregation import com.yahoo.bard.webservice.druid.util.FieldConverterSupplier import com.yahoo.bard.webservice.druid.util.FieldConverters -import com.yahoo.bard.webservice.druid.util.SketchFieldConverter +import com.yahoo.bard.webservice.druid.util.ThetaSketchFieldConverter -import spock.lang.Shared import spock.lang.Specification - +import spock.lang.Unroll /** * Tests the code implemented by MetricMaker, using a stub for code that is meant to be implemented by the children. * Primarily tests the sad paths. The specifications of the children of MetricMaker test the various happy paths. @@ -26,12 +31,86 @@ import spock.lang.Specification class MetricMakerSpec extends Specification { static final String METRIC_NAME = "metric name" + static final int DEPENDENT_METRICS = 1 + + static final FieldConverters originalConverter = FieldConverterSupplier.sketchConverter + static final FieldConverters CONVERTER = new ThetaSketchFieldConverter() + + MetricDictionary dictionary = new MetricDictionary() + + static LogicalMetric longSumMetric + static PostAggregation longSumFieldAccessor + static LogicalMetric squareMetric + static PostAggregation number + + static LogicalMetric sketchAggregationMetric + static PostAggregation sketchFieldAccessor + static LogicalMetric sketchEstimateMetric + static LogicalMetric sketchUnionMetric + static LogicalMetric sketchUnionEstimateMetric + + static LogicalMetric constantMetric + + MetricMaker maker = getMakerInstance() + + def setupSpec() { + + String sketchName = "all_users" + String sketchUnionName = "union_users" + String constantName = "constant1" + String sumName = "longSum" + String squareName = "square" + + number = new ConstantPostAggregation(constantName, 1.0) + FieldConverterSupplier.sketchConverter = CONVERTER + + TemplateDruidQuery queryTemplate + queryTemplate = new TemplateDruidQuery([] as Set, [number] as Set) + constantMetric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), constantName) + + Aggregation longSum = new LongSumAggregation(sumName, "columnName") + queryTemplate = new TemplateDruidQuery([longSum] as Set, [] as Set) + longSumMetric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), longSum.name) + + longSumFieldAccessor = new FieldAccessorPostAggregation(longSum) + PostAggregation square = new ArithmeticPostAggregation(squareName, MULTIPLY, [longSumFieldAccessor, longSumFieldAccessor]) + + queryTemplate = new TemplateDruidQuery([longSum] as Set, [square] as Set) + squareMetric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), square.name) - MetricMaker maker - @Shared - FieldConverters converter = FieldConverterSupplier.sketchConverter + // Theta Sketches + Aggregation sketchAggregation = new ThetaSketchAggregation(sketchName, "columnName", 16000) + queryTemplate = new TemplateDruidQuery([sketchAggregation] as Set, [] as Set) + sketchAggregationMetric = new LogicalMetric(queryTemplate, new SketchRoundUpMapper(), sketchAggregation.name) - private final LogicalMetric METRIC = new LogicalMetric( + PostAggregation sketchEstimateAggregation = CONVERTER.asSketchEstimate(sketchAggregation) + + sketchFieldAccessor = sketchEstimateAggregation.getField() + + queryTemplate = new TemplateDruidQuery([sketchAggregation] as Set, [sketchEstimateAggregation] as Set) + sketchEstimateMetric = new LogicalMetric(queryTemplate, new SketchRoundUpMapper(), sketchEstimateAggregation.name) + + PostAggregation sketchSetAggregation = new ThetaSketchSetOperationPostAggregation( + sketchUnionName, + UNION, + [sketchFieldAccessor, sketchFieldAccessor] + ) + queryTemplate = new TemplateDruidQuery([sketchAggregation] as Set, [sketchSetAggregation] as Set) + sketchUnionMetric = new LogicalMetric(queryTemplate, new SketchRoundUpMapper(), sketchSetAggregation.name) + + PostAggregation sketchSetEstimate = CONVERTER.asSketchEstimate(sketchSetAggregation) + queryTemplate = new TemplateDruidQuery([sketchAggregation] as Set, [sketchSetEstimate] as Set) + sketchUnionEstimateMetric = new LogicalMetric(queryTemplate, new SketchRoundUpMapper(), sketchSetEstimate.name) + } + + def cleanupSpec() { + FieldConverterSupplier.sketchConverter = originalConverter + } + + def setup(){ + } + + private static final LogicalMetric DEFAULT_METRIC = new LogicalMetric( new TemplateDruidQuery([] as Set, [] as Set), new NoOpResultSetMapper(), "no name", @@ -44,16 +123,16 @@ class MetricMakerSpec extends Specification { * and returned every time this method is invoked. * @return A new instance of the MetricMaker under test. */ - MetricMaker getInstance(){ - new MetricMaker(new MetricDictionary()) { + MetricMaker getMakerInstance(){ + new MetricMaker(dictionary) { @Override protected LogicalMetric makeInner(String metricName, List dependentMetrics) { - METRIC + DEFAULT_METRIC } @Override protected int getDependentMetricsRequired() { - 1 + DEPENDENT_METRICS } } } @@ -81,23 +160,13 @@ class MetricMakerSpec extends Specification { } } - def setup(){ - maker = getInstance() - //Initializing the Sketch field converter - FieldConverterSupplier.sketchConverter = new SketchFieldConverter(); - } - - def cleanupSpec() { - FieldConverterSupplier.sketchConverter = converter - } - def "When a metric has too few dependent metrics, an exception is thrown."(){ given: "One fewer dependent metrics than this metric depends on" - List dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()-1) - maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames)) + List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS-1) + dictionary.putAll(makeEmptyMetrics(dependentMetricNames)) when: - maker.make(METRIC_NAME, dependentMetricNames) + getMakerInstance().make(METRIC_NAME, dependentMetricNames) then: thrown(IllegalArgumentException) @@ -105,8 +174,8 @@ class MetricMakerSpec extends Specification { 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 dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()-1) - maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames)) + List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS-1) + dictionary.putAll(makeEmptyMetrics(dependentMetricNames)) dependentMetricNames.add("I don't exist!") when: @@ -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 dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()+1) + maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames)) - def "getNumericField returns a numeric post aggregation unchanged."(){ - given: "A logical metric containing a constant post aggregation" - PostAggregation number = new ConstantPostAggregation("constant1", 1.0) - TemplateDruidQuery queryTemplate = new TemplateDruidQuery([] as Set, [number] as Set) - LogicalMetric constantMetric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), "constant1") - - and: "The maker, with populated metric dictionary" - maker.metrics.add(constantMetric) + when: + maker.make(METRIC_NAME, dependentMetricNames) - expect: - maker.getNumericField("constant1") == number + then: + thrown(IllegalArgumentException) } - def """Given the metric name of an aggregation metric field, getNumericField wraps the - aggregation in a field accessor and returns the new PostAggregation."""(){ - given: "A logical metric containing a metric aggregation" - Aggregation longSumAggregation = new LongSumAggregation("num_users_sum", "num_users") - TemplateDruidQuery queryTemplate = new TemplateDruidQuery([longSumAggregation] as Set, [] as Set) - LogicalMetric longSumMetric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), "num_users_sum") - - and: "The maker, with populated metric dictionary" - maker.metrics.add(longSumMetric) - - and: "The expected post aggregation: a field access wrapping the long sum aggregation" - PostAggregation longSumFieldAccess = new FieldAccessorPostAggregation(longSumAggregation) - + @Unroll + def "getNumericField returns a numeric metric #expected.type for metric named: #fromMetric.name "() { + setup: expect: - maker.getNumericField("num_users_sum") == longSumFieldAccess + MetricMaker.getNumericField(fromMetric.metricField) == expected + + where: + fromMetric | expected + constantMetric | constantMetric.metricField + squareMetric | squareMetric.metricField + longSumMetric | longSumFieldAccessor + sketchEstimateMetric | sketchEstimateMetric.metricField + sketchAggregationMetric | sketchEstimateMetric.metricField + sketchUnionMetric | sketchUnionEstimateMetric.metricField + sketchUnionEstimateMetric | sketchUnionEstimateMetric.metricField } - def "Given a sketch aggregation, getNumericField wraps the sketch in a sketch estimate."(){ - given: "A sketch aggregation" - Aggregation sketchMerge = new SketchMergeAggregation("all_users", "users", 16000) - TemplateDruidQuery queryTemplate = new TemplateDruidQuery([sketchMerge] as Set, [] as Set) - LogicalMetric metric = new LogicalMetric(queryTemplate, new NoOpResultSetMapper(), "all_users", "metric") - - and: "The maker, with populated metric dictionary" - MetricMaker maker = getInstance() - maker.metrics.add(metric) - - and: "The expected post aggregation: A sketch estimate wrapping the sketch aggregation." - PostAggregation sketchEstimate = FieldConverterSupplier.sketchConverter.asSketchEstimate(sketchMerge) - + @Unroll + def "getSketchField returns a sketch aggregation of type #expected.type for metric named: #fromMetric.name "() { + setup: expect: - maker.getNumericField("all_users") == sketchEstimate - + MetricMaker.getSketchField(fromMetric.metricField) == expected + + where: + fromMetric | expected + sketchEstimateMetric | sketchFieldAccessor + sketchAggregationMetric | sketchFieldAccessor + sketchUnionMetric | sketchUnionMetric.metricField + sketchUnionEstimateMetric | sketchUnionMetric.metricField } - 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 dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired()+1) - maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames)) - + @Unroll + def "getSketchField throws error when given a non sketch based field #fromMetric.name "() { + setup: when: - maker.make(METRIC_NAME, dependentMetricNames) + MetricMaker.getSketchField(fromMetric.metricField) then: thrown(IllegalArgumentException) + + where: + fromMetric << [constantMetric, squareMetric, longSumMetric] } }