Finalize fields in postaggs #2883

jaehc · 2016-04-26T10:28:31Z

According to #2433, automatically finalizing aggregators in a post aggregator sounds really good to druid client programs.
I gave it a try to make it work and it seems good. Now, you can write a query like the below. However, there are some duplicate code blocks here and there. Any comments on refactoring would be welcomed.

  "aggregations": [
     {"type": "hyperUnique", "fieldName": "user_unique", "name" : "user_unique"}
  ],
  "postAggregations" : [
     { "type" : "finalizingFieldAccess", "name": "user_unique", "fieldName" : "user_unique" }
  ],

instead of

  "aggregations": [
     {"type": "hyperUnique", "fieldName": "user_unique", "name" : "user_unique"}
  ],
  "postAggregations" : [
     { "type" : "hyperUniqueCardinality", "name": "user_unique", "fieldName" : "user_unique" }
  ],

This change is

fjy · 2016-05-04T22:38:47Z

Thanks @jaehc, we'll try to get to reviewing this soon. We are really backlogged on PRs right now.

jaehc · 2016-08-24T04:49:25Z

rebased to master

fjy · 2016-08-24T23:54:08Z

processing/src/main/java/io/druid/jackson/AggregatorsModule.java

@@ -84,6 +85,7 @@ public AggregatorsModule()
  @JsonSubTypes(value = {
      @JsonSubTypes.Type(name = "arithmetic", value = ArithmeticPostAggregator.class),
      @JsonSubTypes.Type(name = "fieldAccess", value = FieldAccessPostAggregator.class),
+      @JsonSubTypes.Type(name = "finalFieldAccess", value = FinalFieldAccessPostAggregator.class),


can we call this FinalizingFieldAccessPostAggregator?

I will handle it.

fjy · 2016-10-11T00:27:34Z

@nishantmonu51 can you help with review here?

nishantmonu51 · 2016-10-20T04:32:00Z

processing/src/main/java/io/druid/query/Queries.java

@@ -55,6 +61,18 @@ public static void verifyAggregations(
            "Missing fields [%s] for postAggregator [%s]", missing, postAgg.getName()
        );
        Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName());
+
+        if (postAgg instanceof HasDependentAggFactories) {


this probably will not work when a dependent post agg is used within Arithmetic post aggregator.
We need to have a better way to inject aggregators instead of using instanceOf.

A Ut for a finalizing post aggregator with arithmetic post aggregator has beed added. It seems work, but do you mean this case or other situation? Of course, I agree that we can use better ways to get aggregators injected if some. Would you give me some recommandation for this?

I believe you are referring to testComputedInArithmeticPostAggregator.
there its only testing computation instead of dependency injection.
can you add/modify the test to call Queries.prepareAggregations() for arithmetic aggregator and verify that all dependencies are set properly ?

@nishantmonu51 You're right. Injections of post aggs in Arithmetic post aggregator does not work because jackson injects the post aggs while creating an instance which means there is no chance to go through Queries.prepareAggregations()

@nishantmonu51 I've made ArithmeticPostAggregator implements HasDependentAggFactories and modify a existing Ut for that(testIngestAndQueryWithArithmeticPostAggregator). It seems work so far now. But, do you think things are getting ugly?

yeah, looking at this further, there seems to be other aggregators too which act as a wrapper for other postAggs e.g DoubleGreatestPostAggregator, LongGreatestPostAggregator etc.

To make it more cleaner, I think it might just be simpler to add new method setDependentAggFactories in interface PostAggregator itself. Any implementation not having any dependent post aggregator will have a no-op implementation.

@nishantmonu51 It makes sense to me, I will add setDependentAggFactories in PostAggregator and maybe I guess we would probably need another contextual information later and think about it then.

nishantmonu51 · 2016-12-09T20:15:17Z

@jaehc: can you address #2883 (comment) ?

fjy · 2016-12-21T22:28:48Z

ping @jaehc

jaehc · 2016-12-25T08:45:57Z

@fjy @nishantmonu51 sorry for being late. I will handle it

jaehc · 2017-01-04T02:35:24Z

@nishantmonu51
PostAggregator interface now has got to have decorate() which opens flexibility of implementing the computation with more contextual information (e.g. injecting aggregator factory dependencies).
Would you mind going though the idea of decorate()? We are using this interface in our local fork indeed.

nishantmonu51 · 2017-02-05T12:45:40Z

LGTM, with the changes for decorate. @fjy Can you also review ?

nishantmonu51 · 2017-02-05T12:50:00Z

updated the master comment to change finalFieldAccess -> finalizingFieldAccess

nishantmonu51

👍

drcrallen · 2017-02-05T15:45:45Z

Incompatibilities need called out in the master comment, please

jaehc · 2017-02-07T02:05:43Z

@nishantmonu51 Thanks for giving a review. Is there somethign I need to handle about this PR? By the way, what is the master comment you mentioned?

drcrallen · 2017-02-07T02:14:38Z

@jaehc master comment is the top comment in the issue at #2883 (comment)

gianm · 2017-02-07T03:23:22Z

Taking a look.

gianm

@jaehc thanks for the contribution. I just had some small comments and a request to add Comparator support too.

gianm · 2017-02-07T03:30:15Z

processing/src/main/java/io/druid/query/aggregation/PostAggregator.java

@@ -35,4 +35,6 @@
  public Object compute(Map<String, Object> combinedAggregators);

  public String getName();
+
+  public PostAggregator decorate(Map<String, AggregatorFactory> aggregators);


Could you please add a javadoc to this explaining what it's supposed to do, and what aggregators is expected to contain? It's not clear from the method type signature how this would work.

You're right. It needs more explanation. I put some comments on it.

gianm · 2017-02-07T03:35:19Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+  @Override
+  public FinalizingFieldAccessPostAggregator decorate(final Map<String, AggregatorFactory> aggregators)
+  {
+    return new FinalizingFieldAccessPostAggregator(name, fieldName) {


This should override getComparator too, so the post-aggregator can be sorted on. This should be possible by using the comparator of aggregators.get(fieldName).getComparator() if that exists, and Ordering.natural().nullsFirst() otherwise (hoping for the best).

That sound good to me. But, why do we use Ordering.natural().nullsFirst() other than only Ordering.natural()?

Just in case there's a null return value from a postaggregator, I guess.

gianm · 2017-02-07T03:38:10Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+  public String toString()
+  {
+    return "FinalizingFieldAccessPostAggregator{" +
+           "name'" + name + '\'' +


name=' is missing the equals sign

gianm · 2017-02-07T03:38:54Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+    return result;
+  }
+
+  public static FinalizingFieldAccessPostAggregator buildDecorated(String name,


@VisibleForTesting

gianm · 2017-02-07T04:07:55Z

processing/src/main/java/io/druid/query/groupby/GroupByQuery.java

@@ -120,12 +120,15 @@ public GroupByQuery(
      Preconditions.checkArgument(spec != null, "dimensions has null DimensionSpec");
    }
    this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.<AggregatorFactory>of() : aggregatorSpecs;
-    this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.<PostAggregator>of() : postAggregatorSpecs;
+    this.postAggregatorSpecs = Queries.prepareAggregations(


Seems like postAggregatorSpecs == null ? ImmutableList.<PostAggregator>of() : postAggregatorSpecs is a common pattern, might as well have Queries.prepareAggregations do this.

Is there somewhere to apply it other than the line number of 125?

I meant, Queries.prepareAggregations could do this… return an empty list if postAggregatorSpecs is null. This is minor thing and not really necessary though.

gianm · 2017-02-07T04:08:26Z

...g/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java

+import java.util.List;
+import java.util.Map;
+
+public class FinalizingFieldAccessPostAggregatorTest


Please add a test for the comparator too.

done with testComparatorsWithFinalizing and the below Ut.

gianm · 2017-02-07T04:09:43Z

processing/src/main/java/io/druid/query/Queries.java

 import java.util.Set;

 /**
 */
 public class Queries
 {
-  public static void verifyAggregations(
+  public static List<PostAggregator> decorate(List<PostAggregator> postAggs,


minor: name this decoratePostAggregators? Or move to PostAggregators.decorate? The name "Queries.decorate" sounds a little too generic for what this does.

That sound good to me. It was somewhat obscure. For now, I rather lean to rename it. But, when we find out another things doing stuff with post aggregators, it would be good to be in a separated class like PostAggregators.decorate()

jaehc

A patch to address comments.

jaehc · 2017-02-09T03:00:46Z

processing/src/main/java/io/druid/query/groupby/GroupByQuery.java

@@ -120,12 +120,15 @@ public GroupByQuery(
      Preconditions.checkArgument(spec != null, "dimensions has null DimensionSpec");
    }
    this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.<AggregatorFactory>of() : aggregatorSpecs;
-    this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.<PostAggregator>of() : postAggregatorSpecs;
+    this.postAggregatorSpecs = Queries.prepareAggregations(


Is there somewhere to apply it other than the line number of 125?

jaehc · 2017-02-09T03:01:32Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+  public String toString()
+  {
+    return "FinalizingFieldAccessPostAggregator{" +
+           "name'" + name + '\'' +


jaehc · 2017-02-09T03:01:37Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+    return result;
+  }
+
+  public static FinalizingFieldAccessPostAggregator buildDecorated(String name,


jaehc · 2017-02-09T03:06:45Z

processing/src/main/java/io/druid/query/Queries.java

 import java.util.Set;

 /**
 */
 public class Queries
 {
-  public static void verifyAggregations(
+  public static List<PostAggregator> decorate(List<PostAggregator> postAggs,


That sound good to me. It was somewhat obscure. For now, I rather lean to rename it. But, when we find out another things doing stuff with post aggregators, it would be good to be in a separated class like PostAggregators.decorate()

jaehc · 2017-02-10T14:19:32Z

processing/src/main/java/io/druid/query/aggregation/PostAggregator.java

@@ -35,4 +35,6 @@
  public Object compute(Map<String, Object> combinedAggregators);

  public String getName();
+
+  public PostAggregator decorate(Map<String, AggregatorFactory> aggregators);


You're right. It needs more explanation. I put some comments on it.

jaehc · 2017-02-10T14:31:33Z

...ssing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java

+  @Override
+  public FinalizingFieldAccessPostAggregator decorate(final Map<String, AggregatorFactory> aggregators)
+  {
+    return new FinalizingFieldAccessPostAggregator(name, fieldName) {


That sound good to me. But, why do we use Ordering.natural().nullsFirst() other than only Ordering.natural()?

…gregations

…face.

1. Implements getComparator in FinalizingFieldAccessPostAggregator and add Uts for it 2. Some minor changes like renaming a method name.

gianm · 2017-02-13T21:45:49Z

@jaehc thanks, re-reviewing.

gianm

@jaehc, LGTM after fixing conflicts; looks like they are all from #3899 which added cache keys to post-aggregators.

gianm · 2017-02-21T19:52:41Z

I will take a look at resolving conflicts for this patch for 0.10.0.

gianm · 2017-02-22T00:31:06Z

Opened #3957 with conflicts fixed, otherwise unchanged.

fjy added the Improvement label Aug 24, 2016

fjy added this to the 0.9.3 milestone Aug 24, 2016

fjy reviewed Aug 24, 2016
View reviewed changes

nishantmonu51 requested changes Oct 20, 2016

View reviewed changes

gianm assigned fjy and nishantmonu51 Nov 29, 2016

nishantmonu51 added the Incompatible label Feb 5, 2017

nishantmonu51 approved these changes Feb 5, 2017

View reviewed changes

gianm assigned gianm and unassigned fjy Feb 7, 2017

gianm reviewed Feb 7, 2017

View reviewed changes

jaehc commented Feb 10, 2017

View reviewed changes

jaehc added 3 commits February 11, 2017 01:19

initial commits for finalizeFieldAccess #2433

b531ab1

fix some bugs to run a query

77f0d2e

change name of method Queries.verifyAggregations to Queries.prepareAg…

59bc8ae

…gregations

jaehc added 10 commits February 11, 2017 01:19

add Uts

8468199

fix Ut failures

4b07db9

rebased to master

d4dcd2c

address comments and add a Ut for arithmetic post aggregators

c3b4fe9

rebased to the master

7c1a4fb

address the comment of injection within arithmetic post aggregator

e362612

address comments and introduce decorate() in the PostAggregator inter…

74aa554

…face.

Address comments.

4e9aa9e

1. Implements getComparator in FinalizingFieldAccessPostAggregator and add Uts for it 2. Some minor changes like renaming a method name.

Fix a code style mismatch.

d773990

Rebased to the master

2f755f6

gianm reviewed Feb 13, 2017

View reviewed changes

gianm mentioned this pull request Feb 21, 2017

Finalize fields in postaggs #3957

Merged

gianm closed this Feb 22, 2017

clambertus unassigned gianm and nishantmonu51 Jul 6, 2018

Finalize fields in postaggs #2883

Finalize fields in postaggs #2883

Conversation

jaehc commented Apr 26, 2016 • edited by nishantmonu51 Loading

fjy commented May 4, 2016

jaehc commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjy commented Oct 11, 2016

Choose a reason for hiding this comment

jaehc Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

nishantmonu51 Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaehc Dec 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishantmonu51 commented Dec 9, 2016

fjy commented Dec 21, 2016

jaehc commented Dec 25, 2016

jaehc commented Jan 4, 2017

nishantmonu51 commented Feb 5, 2017

nishantmonu51 commented Feb 5, 2017

nishantmonu51 left a comment

Choose a reason for hiding this comment

drcrallen commented Feb 5, 2017

jaehc commented Feb 7, 2017

drcrallen commented Feb 7, 2017

gianm commented Feb 7, 2017

gianm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaehc 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

gianm commented Feb 13, 2017

gianm left a comment

Choose a reason for hiding this comment

gianm commented Feb 21, 2017

gianm commented Feb 22, 2017

jaehc commented Apr 26, 2016 •

edited by nishantmonu51

Loading

jaehc Oct 21, 2016 •

edited

Loading

nishantmonu51 Nov 1, 2016 •

edited

Loading

jaehc Dec 25, 2016 •

edited

Loading