Use GroupBy V2 as default #3953

jon-wei · 2017-02-17T22:25:38Z

Makes GroupBy V2 the default and updates some unit tests to work with V2

b-slim · 2017-02-17T22:32:51Z

processing/src/main/java/io/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java

@@ -318,6 +318,7 @@ private static ValueExtractFunction makeValueExtractFunction(
  )
  {
    final boolean includeTimestamp = GroupByStrategyV2.getUniversalTimestamp(query) == null;
+    //final boolean includeTimestamp = true;


thanks, deleted this

gianm · 2017-02-18T01:18:50Z

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

+                          )
+                      );
+                    } catch (Exception e) {
+                      Assert.fail(e.getMessage());


Assert.fail inside a callback like this seems kinda weird -- usually asserts run inside the main test code. Would it work if this is just a re-throw? Like throw Throwables.propagate(e)

Changed to Throwables.propagate(e)

gianm · 2017-02-18T01:28:38Z

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

@@ -112,6 +133,45 @@ public GroupByTimeseriesQueryRunnerTest(QueryRunner runner)
    super(runner, false);
  }

+  // GroupBy handles timestamps differently when granularity is ALL


Why is this override needed? Do groupBy v1 and v2 behave differently here?

GroupBy v1 and v2 behave the same in this case, when the query granularity is set to ALL, the result timestamps will contain the earliest timestamp in the query interval, which differs from Timeseries

Prior to this patch, testFullOnTimeseriesMaxMin() was not executing mergeResults() on GroupBy V1, so the timestamp wasn't truncated to the ALL granularity and the results matched the Timeseries results exactly.

gianm

LGTM

himanshug · 2017-02-22T22:20:15Z

@gianm @jon-wei
somewhat related to this PR....

after this PR our builds for hadoop indexer test failed to run the groupBy query and failed with exception message "Grouping resources exhausted" from https://github.com/druid-io/druid/blob/master/processing/src/main/java/io/druid/query/groupby/epinephelinae/GroupByMergingQueryRunnerV2.java#L301 with no indication of what the problem was. I added some disk space by druid.query.groupBy.maxOnDiskStorage and the problem disappeared.
that said, it might not be so simple for a regular druid user to understand.
should we change https://github.com/druid-io/druid/blob/master/processing/src/main/java/io/druid/query/groupby/epinephelinae/SpillingGrouper.java#L119 to

throw new ISE(ex, "no space to spill results, please increase druid.query.groupBy.maxOnDiskStorage or druid.processing.buffer.sizeBytes");

gianm · 2017-02-22T22:29:02Z

Yeah that message could be better. I think nothing else other than temporary-storage-full could cause that, so might as well add that to the error message.

gianm · 2017-02-22T22:29:41Z

There's a similar message in GroupByRowProcessor that could be changed too

himanshug · 2017-02-23T00:17:53Z

@gianm wondering why would we modify the message at both places instead of throwing exception with correct message in SpillingGrouper ... is there a case when throwing exception there is not desirable?

gianm · 2017-02-23T00:34:06Z

@himanshug I guess I avoided exceptions because grouper.aggregate returns false normally and I didn't want to have the overhead of exceptions in that code path. For example that's how BufferGrouper tells SpillingGrouper that it needs to be spilled. Usually I try to avoid exceptions for control flow in query code like that.

How about changing the return type from boolean to something that includes a message or a reason?

…ods. Includes two fixes: - groupBy v2 now ignores chunkPeriod, since it wouldn't have helped anyway (its mergeResults returns a lazy sequence) and it generates incorrect results. - Fix chunkPeriod handling for periods of irregular length, like "P1M" or "P1Y". Also includes doc and test fixes: - groupBy v1 was no longer being tested by GroupByQueryRunnerTest since apache#3953, now it is once again. - chunkPeriod documentation was misleading due to its checkered past. Updated it to be more accurate.

…ods. (#4004) * Ignore chunkPeriod for groupBy v2, fix chunkPeriod for irregular periods. Includes two fixes: - groupBy v2 now ignores chunkPeriod, since it wouldn't have helped anyway (its mergeResults returns a lazy sequence) and it generates incorrect results. - Fix chunkPeriod handling for periods of irregular length, like "P1M" or "P1Y". Also includes doc and test fixes: - groupBy v1 was no longer being tested by GroupByQueryRunnerTest since #3953, now it is once again. - chunkPeriod documentation was misleading due to its checkered past. Updated it to be more accurate. * Remove unused import. * Restore buffer size.

…ods. (apache#4004) * Ignore chunkPeriod for groupBy v2, fix chunkPeriod for irregular periods. Includes two fixes: - groupBy v2 now ignores chunkPeriod, since it wouldn't have helped anyway (its mergeResults returns a lazy sequence) and it generates incorrect results. - Fix chunkPeriod handling for periods of irregular length, like "P1M" or "P1Y". Also includes doc and test fixes: - groupBy v1 was no longer being tested by GroupByQueryRunnerTest since apache#3953, now it is once again. - chunkPeriod documentation was misleading due to its checkered past. Updated it to be more accurate. * Remove unused import. * Restore buffer size.

…ods. (#4004) (#4015) * Ignore chunkPeriod for groupBy v2, fix chunkPeriod for irregular periods. Includes two fixes: - groupBy v2 now ignores chunkPeriod, since it wouldn't have helped anyway (its mergeResults returns a lazy sequence) and it generates incorrect results. - Fix chunkPeriod handling for periods of irregular length, like "P1M" or "P1Y". Also includes doc and test fixes: - groupBy v1 was no longer being tested by GroupByQueryRunnerTest since #3953, now it is once again. - chunkPeriod documentation was misleading due to its checkered past. Updated it to be more accurate. * Remove unused import. * Restore buffer size.

jon-wei added the Release Notes label Feb 17, 2017

jon-wei added this to the 0.10.0 milestone Feb 17, 2017

jon-wei requested a review from gianm February 17, 2017 22:25

Use GroupBy V2 as default

a4d4e7e

jon-wei force-pushed the default_groupbyv2 branch from 333a100 to a4d4e7e Compare February 17, 2017 22:26

b-slim reviewed Feb 17, 2017

View reviewed changes

Remove unused line

f5d24f8

jon-wei closed this Feb 17, 2017

jon-wei reopened this Feb 17, 2017

gianm reviewed Feb 18, 2017

View reviewed changes

Change assert to exception propagation

11203f7

jon-wei force-pushed the default_groupbyv2 branch from 40ffcdb to 11203f7 Compare February 18, 2017 02:19

gianm approved these changes Feb 18, 2017

View reviewed changes

b-slim approved these changes Feb 18, 2017

View reviewed changes

b-slim merged commit bc33b68 into apache:master Feb 18, 2017

gianm mentioned this pull request Feb 21, 2017

Finalize fields in postaggs #3957

Merged

This was referenced Feb 28, 2017

Druid 0.10.0 release notes #3944

Closed

Fix groupBy docs to reflect that 'v2' is default. #3993

Merged

gianm mentioned this pull request Mar 4, 2017

Ignore chunkPeriod for groupBy v2, fix chunkPeriod for irregular periods. #4004

Merged

gianm mentioned this pull request Mar 13, 2017

groupBy v2: Better error message when resources are exhausted #4043

Closed

himanshug mentioned this pull request Mar 29, 2017

groupBy v2: Better error message when resources are exhausted. #4046

Merged

jon-wei deleted the default_groupbyv2 branch October 6, 2017 22:22

This was referenced Sep 5, 2021

[Snyk] Security upgrade axios from 0.18.0 to 0.21.3 ajesse11x/incubator-druid#229

Open

[Snyk] Security upgrade axios from 0.21.1 to 0.21.3 terrorizer1980/druid#11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GroupBy V2 as default #3953

Use GroupBy V2 as default #3953

jon-wei commented Feb 17, 2017

b-slim Feb 17, 2017

jon-wei Feb 17, 2017

gianm Feb 18, 2017

jon-wei Feb 18, 2017

gianm Feb 18, 2017

jon-wei Feb 18, 2017 •

edited

gianm left a comment

himanshug commented Feb 22, 2017 •

edited

gianm commented Feb 22, 2017 •

edited

gianm commented Feb 22, 2017

himanshug commented Feb 23, 2017

gianm commented Feb 23, 2017

Use GroupBy V2 as default #3953

Use GroupBy V2 as default #3953

Conversation

jon-wei commented Feb 17, 2017

b-slim Feb 17, 2017

Choose a reason for hiding this comment

jon-wei Feb 17, 2017

Choose a reason for hiding this comment

gianm Feb 18, 2017

Choose a reason for hiding this comment

jon-wei Feb 18, 2017

Choose a reason for hiding this comment

gianm Feb 18, 2017

Choose a reason for hiding this comment

jon-wei Feb 18, 2017 • edited

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

himanshug commented Feb 22, 2017 • edited

gianm commented Feb 22, 2017 • edited

gianm commented Feb 22, 2017

himanshug commented Feb 23, 2017

gianm commented Feb 23, 2017

jon-wei Feb 18, 2017 •

edited

himanshug commented Feb 22, 2017 •

edited

gianm commented Feb 22, 2017 •

edited