First and Last Aggregator by jon-wei · Pull Request #3566 · apache/druid

jon-wei · 2016-10-13T23:02:39Z

Reopening #3226 from @acslk to finish review/merge

fjy · 2016-11-04T20:53:38Z

Still 👍 from me after we resolve merge conflicts

jon-wei · 2016-11-04T21:22:49Z

rebased

fjy · 2016-12-07T01:50:49Z

@nishantmonu51 can u review?

nishantmonu51

left few comments

nishantmonu51 · 2016-12-09T19:19:21Z


+### First / Last aggregator
+
+First and Last aggregator cannot be used in ingestion spec, and should only be specified as part of queries.


can we also document that if the rows are rolled up, the first/last value will be a the first/last value in druid row and not the raw data being ingested.

Added note about rollup behavior

nishantmonu51 · 2016-12-09T19:21:24Z

              public AggregatorFactory apply(String input)
              {
-                return new JavaScriptAggregatorFactory(input, fieldNames, fnAggregate, fnReset, fnCombine, config);
+                return new JavaScriptAggregatorFactory(input, Lists.newArrayList(input), fnCombine, fnReset, fnCombine, config);


seems unrelated to this PR ?

it's needed due to the changes within groupby in the PR

nishantmonu51 · 2016-12-09T19:21:59Z

+  private final LongColumnSelector timeSelector;
+  private final String name;
+
+  long firstTime;


make private ?

made this and others protected, it's accessed by the corresponding AggregatorFactory in getCombiningFactory() when overriding factorize()

nishantmonu51 · 2016-12-09T19:22:06Z

+  private final String name;
+
+  long firstTime;
+  double firstValue;


nishantmonu51 · 2016-12-09T19:31:47Z

+  private final String name;
+
+  long firstTime;
+  long firstValue;


nishantmonu51 · 2016-12-09T19:42:20Z

+  private final String name;
+
+  long lastTime;
+  double lastValue;


nishantmonu51 · 2016-12-09T19:42:57Z

+  {
+    return new DoubleLastAggregator(
+        name, metricFactory.makeFloatColumnSelector(fieldName),
+        metricFactory.makeLongColumnSelector(Column.TIME_COLUMN_NAME)


minor nit: can we pass time column selector before field selector to be consistent with buffered aggregator ?

nishantmonu51 · 2016-12-09T19:43:57Z

+  @Override
+  public String toString()
+  {
+    return "DoubleFirstAggregatorFactory{" +


DoubleLast ?

fixed this name

nishantmonu51 · 2016-12-09T19:45:17Z

+  private final String name;
+
+  long lastTime;
+  long lastValue;


nishantmonu51 · 2016-12-09T19:46:10Z

+  {
+    return new LongLastAggregator(
+        name, metricFactory.makeLongColumnSelector(fieldName),
+        metricFactory.makeLongColumnSelector(Column.TIME_COLUMN_NAME)


minor nit: can we pass time column selector before field selector to be consistent with buffered aggregator ?

nishantmonu51 · 2016-12-09T19:49:47Z

Also, would be nice to add these aggregators to existing integration tests, we can add them to ITTwitterQueryTest and ITWikipediaQueryTest

…stlast

jon-wei · 2016-12-14T20:28:05Z

@nishantmonu51 Addressed your comments aside from the one about integration tests, haven't gotten around to that yet

nishantmonu51

👍 from my side, this can be merged after creating an issue to track integration-test changes, if you want to do that in a separate PR.

jon-wei · 2016-12-16T22:50:37Z

@nishantmonu51 @fjy

I added some first/last aggs to ITWikipediaQueryTest

I didn't add them to ITTwitterQueryTest since I saw the queries there weren't testing the full set of aggs, only using doubleSums generally.

I noticed that some other agg types weren't represented in the integration tests (like the max/min or javascript), and it'd be convenient to have a reference to the raw data for twitterstream and wikipedia_editstream, so I'll open issues for those points

fjy · 2016-12-16T22:53:28Z

👍 after travis

jon-wei · 2016-12-16T22:59:35Z

Opened two new issues re: integration tests

#3783
#3784

* add first and last aggregator * add test and fix * moving around * separate aggregator valueType * address PR comment * add finalize inner query and adjust v1 inner indexing * better test and fixes * java-util import fixes * PR comments * Add first/last aggs to ITWikipediaQueryTest

… volcano planner

jon-wei mentioned this pull request Oct 13, 2016

First and Last Aggregator #3226

Closed

jon-wei force-pushed the acslk-feature-firstlast branch from e5182d5 to 8699fa5 Compare October 13, 2016 23:08

jon-wei added the Feature label Oct 13, 2016

jon-wei added this to the 0.9.3 milestone Oct 13, 2016

acslk added 5 commits November 4, 2016 13:48

add first and last aggregator

dbc1069

add test and fix

759d902

moving around

4d96703

separate aggregator valueType

237b552

address PR comment

78bcfae

acslk added 2 commits November 4, 2016 13:57

add finalize inner query and adjust v1 inner indexing

890c827

better test and fixes

3c069a9

jon-wei force-pushed the acslk-feature-firstlast branch from 8699fa5 to 3c069a9 Compare November 4, 2016 20:58

java-util import fixes

481b526

gianm assigned fjy and nishantmonu51 Nov 29, 2016

nishantmonu51 reviewed Dec 9, 2016

View reviewed changes

Merge remote-tracking branch 'upstream/master' into acslk-feature-fir…

fa528cb

…stlast

jon-wei force-pushed the acslk-feature-firstlast branch from 1e2f53a to 2680177 Compare December 14, 2016 00:02

PR comments

cc25586

jon-wei force-pushed the acslk-feature-firstlast branch from 2680177 to cc25586 Compare December 14, 2016 00:06

nishantmonu51 approved these changes Dec 16, 2016

View reviewed changes

Add first/last aggs to ITWikipediaQueryTest

11e50b6

jon-wei force-pushed the acslk-feature-firstlast branch from f362d19 to 11e50b6 Compare December 16, 2016 22:45

fjy merged commit 2bfcc8a into apache:master Dec 16, 2016

jon-wei deleted the acslk-feature-firstlast branch October 6, 2017 22:19

clambertus unassigned fjy and nishantmonu51 Jul 6, 2018

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3566 Extract common condition from disjunction operands before…

f292524

… volcano planner


		### First / Last aggregator

		First and Last aggregator cannot be used in ingestion spec, and should only be specified as part of queries.

Conversation

jon-wei commented Oct 13, 2016

Uh oh!

fjy commented Nov 4, 2016

Uh oh!

jon-wei commented Nov 4, 2016

Uh oh!

fjy commented Dec 7, 2016

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishantmonu51 commented Dec 9, 2016

Uh oh!

jon-wei commented Dec 14, 2016

Uh oh!

nishantmonu51 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Dec 16, 2016

Uh oh!

fjy commented Dec 16, 2016

Uh oh!

jon-wei commented Dec 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nishantmonu51 left a comment •

edited

Loading