Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

First and Last Aggregator #3566

Merged
merged 11 commits into from
Dec 16, 2016
Merged

First and Last Aggregator #3566

merged 11 commits into from
Dec 16, 2016

Conversation

jon-wei
Copy link
Contributor

@jon-wei jon-wei commented Oct 13, 2016

Reopening #3226 from @acslk to finish review/merge

@jon-wei jon-wei mentioned this pull request Oct 13, 2016
Closed
@jon-wei jon-wei force-pushed the acslk-feature-firstlast branch from e5182d5 to 8699fa5 Compare October 13, 2016 23:08
@jon-wei jon-wei added this to the 0.9.3 milestone Oct 13, 2016
@fjy
Copy link
Contributor

fjy commented Nov 4, 2016

Still 👍 from me after we resolve merge conflicts

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

jon-wei commented Nov 4, 2016

rebased

@fjy
Copy link
Contributor

fjy commented Dec 7, 2016

@nishantmonu51 can u review?

nishantmonu51
nishantmonu51 reviewed Dec 9, 2016
View reviewed changes
Copy link
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left few comments

docs/content/querying/aggregations.md
@@ -76,6 +76,58 @@ Computes the sum of values as 64-bit floating point value. Similar to `longSum`
{ "type" : "longMax", "name" : <output_name>, "fieldName" : <metric_name> }
```

### First / Last aggregator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added note about rollup behavior

processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java
@@ -171,7 +171,7 @@ public AggregatorFactory getMergingFactory(AggregatorFactory other) throws Aggre
@Override
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems unrelated to this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

processing/src/main/java/io/druid/query/aggregation/first/DoubleFirstAggregator.java
private final LongColumnSelector timeSelector;
private final String name;

long firstTime;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make private ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

processing/src/main/java/io/druid/query/aggregation/first/DoubleFirstAggregator.java
private final String name;

long firstTime;
double firstValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private?

processing/src/main/java/io/druid/query/aggregation/first/LongFirstAggregator.java
private final String name;

long firstTime;
long firstValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private ?

processing/src/main/java/io/druid/query/aggregation/last/DoubleLastAggregator.java
private final String name;

long lastTime;
double lastValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private ?

processing/src/main/java/io/druid/query/aggregation/last/DoubleLastAggregatorFactory.java
{
return new DoubleLastAggregator(
name, metricFactory.makeFloatColumnSelector(fieldName),
metricFactory.makeLongColumnSelector(Column.TIME_COLUMN_NAME)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

processing/src/main/java/io/druid/query/aggregation/last/DoubleLastAggregatorFactory.java
@Override
public String toString()
{
return "DoubleFirstAggregatorFactory{" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DoubleLast ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this name

processing/src/main/java/io/druid/query/aggregation/last/LongLastAggregator.java
private final String name;

long lastTime;
long lastValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private.

processing/src/main/java/io/druid/query/aggregation/last/LongLastAggregatorFactory.java
{
return new LongLastAggregator(
name, metricFactory.makeLongColumnSelector(fieldName),
metricFactory.makeLongColumnSelector(Column.TIME_COLUMN_NAME)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nishantmonu51
Copy link
Member

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

@jon-wei jon-wei force-pushed the acslk-feature-firstlast branch from 1e2f53a to 2680177 Compare December 14, 2016 00:02
@jon-wei jon-wei force-pushed the acslk-feature-firstlast branch from 2680177 to cc25586 Compare December 14, 2016 00:06
@jon-wei
Copy link
Contributor Author

jon-wei commented Dec 14, 2016

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

nishantmonu51
nishantmonu51 approved these changes Dec 16, 2016
View reviewed changes
Copy link
Member

@nishantmonu51 nishantmonu51 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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 jon-wei force-pushed the acslk-feature-firstlast branch from f362d19 to 11e50b6 Compare December 16, 2016 22:45
@jon-wei
Copy link
Contributor Author

jon-wei commented Dec 16, 2016

@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
Copy link
Contributor

fjy commented Dec 16, 2016

👍 after travis

@jon-wei
Copy link
Contributor Author

jon-wei commented Dec 16, 2016

Opened two new issues re: integration tests

#3783
#3784

@fjy fjy merged commit 2bfcc8a into apache:master Dec 16, 2016
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
@jon-wei @dgolitsyn
First and Last Aggregator (apache#3566)
17889b0 
* 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
@jon-wei jon-wei deleted the acslk-feature-firstlast branch October 6, 2017 22:19
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022
@navis
apache#3566 Extract common condition from disjunction operands before…
f292524 
… volcano planner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nishantmonu51 nishantmonu51 nishantmonu51 approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jon-wei @fjy @nishantmonu51 @acslk