Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators #1044

xvrl · 2015-01-17T01:13:59Z

Ensure names for aggregators and post-aggregators are unique
Fixes GroupBy with complex metrics fails if there is a post-aggregator with the same name #1045 by not allowing the same name for both post-aggregators and aggregators
Optimizes post-aggregator calculation for groupBy

drcrallen · 2015-02-02T16:23:27Z

Fixes #1045 via metamx@60e065d

nishantmonu51 · 2015-02-03T04:42:41Z

+1

drcrallen · 2015-02-23T16:53:17Z

@xvrl : can this be a 0.7.1 fix?

xvrl · 2015-02-23T17:50:09Z

yup, added it to 0.7.1

On Mon, Feb 23, 2015 at 8:53 AM, Charles Allen notifications@github.com
wrote:

@xvrl https://github.com/xvrl : can this be a 0.7.1 fix?

—
Reply to this email directly or view it on GitHub
#1044 (comment).

fjy · 2015-02-25T22:37:53Z

Can I propose a simpler fix of just throwing a meaningful exception when agg/postagg match?

drcrallen · 2015-02-25T22:41:58Z

@fjy : That is possible, but be aware that the current proposed fix overwrites the prior values with new ones. This means that if someone has complex aggregations and some other post-aggregations, the post aggs will overwrite the aggregators in the results set, this saves on ser/de when sending back to the client.

fjy · 2015-03-02T22:48:06Z

@cheddar @himanshug @xvrl Any thoughts on this one? In particular with my last set of comments.

himanshug · 2015-03-03T03:58:43Z

I think I agree with @fjy in keeping it simple and just not allowing aggregator and post-aggregator of same names. Semantics are then simpler to understand/explain.

@drcrallen savings on serde, if required, can be attained by fixing #688

drcrallen · 2015-03-10T18:41:28Z

@cheddar / @fjy / @himanshug / @xvrl
This changes behavior of the query engine and could break existing queries, but now implements the behavior as suggested by @fjy / @himanshug

himanshug · 2015-03-10T21:22:56Z

...src/test/java/io/druid/query/aggregation/histogram/ApproximateHistogramGroupByQueryTest.java

@@ -214,4 +214,64 @@ public void testGroupByWithApproximateHistogramAgg()
    Iterable<Row> results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
    TestHelper.assertExpectedObjects(expectedResults, results, "approx-histo");
  }
+
+  @Test(expected = java.lang.IllegalArgumentException.class)


minor nit, but java.lang isn't really required.

Fixed, and in 1 other spot.

himanshug · 2015-03-10T21:25:17Z

LGTM besides the minor comment.

drcrallen · 2015-03-10T21:35:47Z

To be clear, the "fix" for the issue is still in the PR, so the underlying logic can support group by queries where the aggregate and post aggregate are named the same but now they simply refuse to.

xvrl · 2015-03-10T21:37:31Z

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

@@ -64,6 +68,40 @@ public String apply(AggregatorFactory input)
        );
        combinedAggNames.add(postAgg.getName());
      }
+
+      final Collection<String> aggAndPostAgg = Sets.intersection(
+          Sets.<String>newHashSet(


isn't this the same as combinedAggNames?

the initial value I mean

Yes, I had originally taken this as a separate group of code to separate functionality, but it actually doesn't look too bad when I put them in the same logic area.

coveralls · 2015-03-10T21:52:43Z

Coverage decreased (-0.02%) to 52.11% when pulling 4393876087cf55b0fb64fae4a44bab5cc3040704 on metamx:groupby-postagg-naming-bug into c7ec8ba on druid-io:master.

drcrallen · 2015-03-10T21:56:52Z

Modified this a little to also do the same check for aggregators (that aggregator names must be unique)

coveralls · 2015-03-10T22:32:32Z

Coverage increased (+0.01%) to 52.13% when pulling 92de15d6b2fa7ae3a738e8515046bdc68f8a7883 on metamx:groupby-postagg-naming-bug into 9f242ed on druid-io:master.

coveralls · 2015-03-10T22:37:14Z

Coverage increased (+0.06%) to 52.18% when pulling e92e664800b4e7d9b37940e1e71c6e30dbd6fb3c on metamx:groupby-postagg-naming-bug into 9f242ed on druid-io:master.

xvrl · 2015-03-10T22:55:52Z

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

+        final MapBasedRow inputRow = (MapBasedRow) input;
+        final Map<String, Object> values = Maps.newHashMap(inputRow.getEvent());
+        for (AggregatorFactory agg : query.getAggregatorSpecs()) {
+          if (postNames.contains(agg.getName())) {


we can remove this since we don't allow same names.

Defaulted to pre-PR behavior for this.

xvrl · 2015-03-10T23:01:20Z

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

+                  query.getDimensions(),
+                  query.getAggregatorSpecs(),
+                  // Don't do post aggs until the end of this method. ISSUE #1045
+                  ImmutableList.<PostAggregator>of(),


this comment is probably misleading now, since this is more of an optimization rather than fixing a bug now.

coveralls · 2015-03-10T23:18:09Z

Coverage decreased (-0.04%) to 52.08% when pulling f6778651a0bf42525800bc25e7ea31a76376eaf2 on metamx:groupby-postagg-naming-bug into 9f242ed on druid-io:master.

coveralls · 2015-03-10T23:20:22Z

Coverage increased (+0.02%) to 52.14% when pulling f6778651a0bf42525800bc25e7ea31a76376eaf2 on metamx:groupby-postagg-naming-bug into 9f242ed on druid-io:master.

xvrl · 2015-03-10T23:43:57Z

processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java

@@ -1152,6 +1191,62 @@ public void testGroupByWithOrderLimit4()
  }

  @Test
+  public void testPostAggHavingSpec()


this only tests post-aggregators in the context of merging results.
Can we add a separate one that does it without merging, i.e. that checks the results of GroupByQueryRunnerTestHelper.runQuery directly?

coveralls · 2015-03-10T23:46:47Z

Coverage decreased (-0.02%) to 52.1% when pulling 1ee7e4898a7b246e71298ca6f5513904e84b7cd1 on metamx:groupby-postagg-naming-bug into 9f242ed on druid-io:master.

* add test for same-name groupBy hyperUniques post-agg * add test for same-name post-agg in groupby with approx histogram * Fixes apache#1045 * Throws an error if post aggs and aggs do not have unique names * Add more groupBy tests for Having filters

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators

xvrl force-pushed the groupby-postagg-naming-bug branch from 726d272 to 743316d Compare January 17, 2015 01:17

drcrallen mentioned this pull request Jan 17, 2015

GroupBy with complex metrics fails if there is a post-aggregator with the same name #1045

Closed

xvrl added the Bug label Jan 17, 2015

drcrallen force-pushed the groupby-postagg-naming-bug branch from 743316d to 60e065d Compare January 29, 2015 02:16

fjy force-pushed the master branch 2 times, most recently from 8b0ec82 to d05032b Compare February 1, 2015 04:57

xvrl added this to the 0.7.1 milestone Feb 23, 2015

xvrl changed the title ~~Tests for groupBy with post-aggregators on complex metrics with the same name~~ Tests + fix for groupBy with post-aggregators on complex metrics with the same name Feb 23, 2015

drcrallen mentioned this pull request Feb 25, 2015

Various unit tests, minor fixes, and dead code removal #1148

Closed

drcrallen force-pushed the groupby-postagg-naming-bug branch from 60e065d to 4393876 Compare March 10, 2015 18:40

himanshug reviewed Mar 10, 2015
View reviewed changes

drcrallen force-pushed the groupby-postagg-naming-bug branch from 4393876 to e92e664 Compare March 10, 2015 21:34

xvrl reviewed Mar 10, 2015
View reviewed changes

drcrallen force-pushed the groupby-postagg-naming-bug branch from e92e664 to 92de15d Compare March 10, 2015 21:54

xvrl reviewed Mar 10, 2015
View reviewed changes

drcrallen force-pushed the groupby-postagg-naming-bug branch from 92de15d to 89af08c Compare March 10, 2015 23:01

xvrl reviewed Mar 10, 2015
View reviewed changes

drcrallen force-pushed the groupby-postagg-naming-bug branch from 89af08c to f677865 Compare March 10, 2015 23:05

xvrl changed the title ~~Tests + fix for groupBy with post-aggregators on complex metrics with the same name~~ Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators Mar 10, 2015

xvrl reviewed Mar 10, 2015
View reviewed changes

drcrallen force-pushed the groupby-postagg-naming-bug branch from 1fd6b33 to 217e674 Compare March 11, 2015 00:12

xvrl added a commit that referenced this pull request Mar 11, 2015

Merge pull request #1044 from metamx/groupby-postagg-naming-bug

c97fbdf

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators

xvrl merged commit c97fbdf into apache:master Mar 11, 2015

xvrl deleted the groupby-postagg-naming-bug branch March 11, 2015 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators #1044

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators #1044

xvrl commented Jan 17, 2015

drcrallen commented Feb 2, 2015

nishantmonu51 commented Feb 3, 2015

drcrallen commented Feb 23, 2015

xvrl commented Feb 23, 2015

fjy commented Feb 25, 2015

drcrallen commented Feb 25, 2015

fjy commented Mar 2, 2015

himanshug commented Mar 3, 2015

drcrallen commented Mar 10, 2015

himanshug Mar 10, 2015

drcrallen Mar 10, 2015

himanshug commented Mar 10, 2015

drcrallen commented Mar 10, 2015

xvrl Mar 10, 2015

xvrl Mar 10, 2015

drcrallen Mar 10, 2015

coveralls commented Mar 10, 2015

drcrallen commented Mar 10, 2015

coveralls commented Mar 10, 2015

coveralls commented Mar 10, 2015

xvrl Mar 10, 2015

drcrallen Mar 10, 2015

xvrl Mar 10, 2015

drcrallen Mar 10, 2015

coveralls commented Mar 10, 2015

coveralls commented Mar 10, 2015

xvrl Mar 10, 2015

drcrallen Mar 10, 2015

coveralls commented Mar 10, 2015

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators #1044

Ensure unique names for aggregators / post-aggregators + optimize groupBy post-aggregators #1044

Conversation

xvrl commented Jan 17, 2015

drcrallen commented Feb 2, 2015

nishantmonu51 commented Feb 3, 2015

drcrallen commented Feb 23, 2015

xvrl commented Feb 23, 2015

fjy commented Feb 25, 2015

drcrallen commented Feb 25, 2015

fjy commented Mar 2, 2015

himanshug commented Mar 3, 2015

drcrallen commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Mar 10, 2015

drcrallen commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 10, 2015

drcrallen commented Mar 10, 2015

coveralls commented Mar 10, 2015

coveralls commented Mar 10, 2015

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

coveralls commented Mar 10, 2015

coveralls commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 10, 2015