Added getMetricField to LogicalMetric #124

michael-mclawhorn · 2016-12-15T16:55:48Z

Simplified calls in maker code so as to not use TDQ where unnecessary
Streamified for loop for making arithmetic operations

cdeszaq

Some concerns about change management, but otherwise the changes look fine.

cdeszaq · 2016-12-15T17:00:03Z

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

-     * @param name  Name of the metric to fetch the template druid query from
-     * @return The template druid query of the metric
-     */
-    protected TemplateDruidQuery getDependentQuery(String name) {


We can't just remove this, since it was protected and in a class that we intended to be extended. We need to @Deprecate it instead.

cdeszaq · 2016-12-15T17:03:36Z

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

+        return names.stream()
+                .map(metrics::get)
+                .map(LogicalMetric::getTemplateDruidQuery)
+                .reduce(TemplateDruidQuery::merge)


Does merge handle null correctly? I know we're collapsing the error check to the end of this stream (which I'm a fan of), but I just want to make sure that the merge will still be working correctly.

The previous implementation calls names.size first and thus blows up on names being null.
Similarly, getDependentQuery wasn't nullsafeing on it's MetricDictionary.get().getTemplateDruidQuery()
So the only open question is whether reduce with a binary function does the thing it's spec says it does.

I didn't look at reduce's contract, so if it's protecting us, we're good 😄

cdeszaq · 2016-12-15T17:08:00Z

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

@@ -187,7 +170,7 @@ protected TemplateDruidQuery getMergedQuery(List<String> names) {
     */
    protected PostAggregation getNumericField(String fieldName) {
        // Get the field
-        MetricField field = getDependentQuery(fieldName).getMetricField(fieldName);
+        MetricField field = metrics.get(fieldName).getMetricField();


This is not the same semantics, is it? In the previous code, it was retrieving an aggregation with a specific name from a metric with that name. Admittedly, the previous code had many assumptions, and I think the new semantics are better, but we should be at least a little careful here, since this is a protected method, meaning that others could be relying on it...

The previous method call did:
foo = metrics.get(fieldName)
bar = return foo.getTemplateDruidQuery
bar.getMetricField(fieldName)

How is this not the same?

Ahh, I see. Makes sense then. Clearly, the previous code was worse, since it was harder to trace 😉

cdeszaq · 2016-12-15T17:08:11Z

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

@@ -212,7 +195,7 @@ protected PostAggregation getNumericField(String fieldName) {
     */
    protected PostAggregation getSketchField(String fieldName) {
        // Get the field
-        MetricField field = getDependentQuery(fieldName).getMetricField(fieldName);
+        MetricField field = metrics.get(fieldName).getMetricField();


ditto my comments on semantic changes

cdeszaq · 2016-12-15T17:10:18Z

...c/main/java/com/yahoo/bard/webservice/data/config/metric/makers/AggregationAverageMaker.java

@@ -61,15 +61,13 @@ public AggregationAverageMaker(MetricDictionary metrics, ZonelessTimeGrain inner
    @Override
    protected LogicalMetric makeInner(String metricName, List<String> dependentMetrics) {
        // Get the Metric that is being averaged over
-        String dependantMetricName = dependentMetrics.get(0);
-        TemplateDruidQuery innerDependentQuery = getDependentQuery(dependantMetricName);
-        MetricField sourceMetric = innerDependentQuery.getMetricField(dependantMetricName);


Is this really the only maker in which we were doing this?

the vast majority of our makes are aggregation makers.
Of the post aggregation makers, sketch makers access this type of logic via the 'getSketchField' method in the base MetricMaker class.
ArithmeticMaker goes through the 'getNumericField' method in the MetricMaker class.
That just leaves daily averages. We have less post aggregation type variety than you might suspect.

Fair enough.

Yay for less variety than you might suspect!

cdeszaq

Just the call-out of the deprecation in the ChangeLog. Otherwise, this looks good. 👍

cdeszaq · 2016-12-15T19:56:50Z

...c/main/java/com/yahoo/bard/webservice/data/config/metric/makers/AggregationAverageMaker.java

@@ -61,15 +61,13 @@ public AggregationAverageMaker(MetricDictionary metrics, ZonelessTimeGrain inner
    @Override
    protected LogicalMetric makeInner(String metricName, List<String> dependentMetrics) {
        // Get the Metric that is being averaged over
-        String dependantMetricName = dependentMetrics.get(0);
-        TemplateDruidQuery innerDependentQuery = getDependentQuery(dependantMetricName);
-        MetricField sourceMetric = innerDependentQuery.getMetricField(dependantMetricName);


Fair enough.

archolewa · 2016-12-16T21:14:12Z

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

+                .map(metrics::get)
+                .map(LogicalMetric::getTemplateDruidQuery)
+                .reduce(TemplateDruidQuery::merge)
+                .orElseGet(() -> {


What you actually want to use here is orElseThrow:

.orElseThrow(() -> { String message = "At least 1 name is needed to merge aggregations"; LOG.error(message); return new IllegalArgumentException(message); }

Ah, yes, that's the one. I knew there was something like that...

archolewa · 2016-12-16T21:15:58Z

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

+                            String message = "At least 1 name is needed to merge aggregations";
+                            LOG.error(message);
+                            throw new IllegalArgumentException(message);
+                        }


Is this bracket indented correctly? Typically, when the opening of a lambda is on the same line as a method, I put the closing bracket on the same line as the closing parens. i.e.:

method(() -> { doStuf(); });

and the closing bracket is on its own line only if the start of the lambda is on its own line:

method( () -> { doStuf(); } );

I don't have a strong opinion either way, I just wanted to know if we had a convention around this.

It's not wrong, just a little different. I think all of the options are pretty much equal, though I do like the more compressed form of stacking things where it's obvious what's closing and isn't stacked too deep if I think hard about it.

Autoformat thinks that the closure shouldn't be double indented because it's a sub block not an argument line continuation. What do you think of what I'm about to post?

I think it's fine.

archolewa · 2016-12-16T21:16:38Z

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

@@ -187,7 +185,7 @@ protected TemplateDruidQuery getMergedQuery(List<String> names) {
     */
    protected PostAggregation getNumericField(String fieldName) {
        // Get the field


This comment is now redundant.

Simplified calls in maker code so as to not use TDQ where unnecessary Streamified for loop for making arithmetic operations

Tweaked indentation.

archolewa · 2016-12-20T17:31:55Z

👍

michael-mclawhorn force-pushed the RefactorMetricFieldAccessor branch 3 times, most recently from daaf6e8 to f1cebbd Compare December 15, 2016 17:02

michael-mclawhorn requested review from cdeszaq and archolewa December 15, 2016 17:03

michael-mclawhorn added NEED 2 REVIEWS REVIEWABLE labels Dec 15, 2016

cdeszaq requested changes Dec 15, 2016

View reviewed changes

cdeszaq approved these changes Dec 15, 2016

View reviewed changes

michael-mclawhorn force-pushed the RefactorMetricFieldAccessor branch from c3a735b to 5865766 Compare December 15, 2016 19:57

archolewa requested changes Dec 16, 2016

View reviewed changes

archolewa added NEED CHANGES NEED 1 REVIEW and removed NEED CHANGES NEED 2 REVIEWS labels Dec 16, 2016

michael-mclawhorn added 4 commits December 20, 2016 09:48

Added getMetricField to LogicalMetric

6ed8584

Simplified calls in maker code so as to not use TDQ where unnecessary Streamified for loop for making arithmetic operations

Restored and deprecated old convenience function.

7c1474a

Added deprecation CHANGELOG

263441f

Added deprecation.

df01060

Tweaked indentation.

michael-mclawhorn force-pushed the RefactorMetricFieldAccessor branch from feada28 to df01060 Compare December 20, 2016 16:17

archolewa approved these changes Dec 20, 2016

View reviewed changes

archolewa added MERGEABLE and removed NEED 1 REVIEW NEED CHANGES labels Dec 20, 2016

cdeszaq merged commit c71725d into master Dec 20, 2016

cdeszaq deleted the RefactorMetricFieldAccessor branch December 20, 2016 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added getMetricField to LogicalMetric #124

Added getMetricField to LogicalMetric #124

michael-mclawhorn commented Dec 15, 2016

cdeszaq left a comment

cdeszaq Dec 15, 2016

michael-mclawhorn Dec 15, 2016

cdeszaq Dec 15, 2016

michael-mclawhorn Dec 15, 2016

cdeszaq Dec 15, 2016

cdeszaq Dec 15, 2016

michael-mclawhorn Dec 15, 2016

cdeszaq Dec 15, 2016

cdeszaq Dec 15, 2016

cdeszaq Dec 15, 2016

michael-mclawhorn Dec 15, 2016

cdeszaq Dec 15, 2016

archolewa Dec 16, 2016

cdeszaq left a comment

cdeszaq Dec 15, 2016

archolewa Dec 16, 2016

cdeszaq Dec 19, 2016

archolewa Dec 16, 2016

cdeszaq Dec 19, 2016

michael-mclawhorn Dec 20, 2016

archolewa Dec 20, 2016

archolewa Dec 16, 2016

archolewa commented Dec 20, 2016

Added getMetricField to LogicalMetric #124

Added getMetricField to LogicalMetric #124

Conversation

michael-mclawhorn commented Dec 15, 2016

cdeszaq 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

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

cdeszaq 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

archolewa commented Dec 20, 2016