Skip to content

Replace AggregatorParsers with namedObject #22397

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
Merged
merged 4 commits into from
Jan 9, 2017
Merged

Replace AggregatorParsers with namedObject #22397

merged 4 commits into from
Jan 9, 2017

Conversation

@nik9000
Copy link
Member

@nik9000 nik9000 commented Dec 31, 2016

Removes AggregatorParsers, replacing all of its functionality with
XContentParser#namedObject.

This is the third bit of payoff from #22003, one less thing to pass
around the entire application.

@nik9000
Copy link
Member Author

nik9000 commented Dec 31, 2016

@imotov or @martijnvg or @javanna, could one of you have a look at this? Another namedObject side effect....

s1monw
s1monw reviewed Jan 9, 2017
View reviewed changes
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

left one comment LGTM otherwise

core/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationBuilder.java Outdated
Copy link
Contributor

@s1monw s1monw Jan 9, 2017

Choose a reason for hiding this comment

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

can we maybe simplify this and just make PipelineAggregationBuilder a subclass of AggregationBuilder and remote that interface? that would be awesome

Copy link
Member Author

@nik9000 nik9000 Jan 9, 2017

Choose a reason for hiding this comment

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

Hmmm. I didn't look at that. I'll have a look! I'd love to not need the interface.

nik9000 added 4 commits January 9, 2017 13:04
@nik9000
Replace AggregatorParsers with namedObject
39ccba0 
Removes `AggregatorParsers`, replacing all of its functionality with
`XContentParser#namedObject`.

This is the third bit of payoff from elastic#22003, one less thing to pass
around the entire application.
@nik9000
Woops
d4f6ec2
@nik9000
Javadoc
5ff347a
@nik9000
Fix after rebase
6c0f4c5
@nik9000
Copy link
Member Author

nik9000 commented Jan 9, 2017

@s1monw I had a look at making PipelineAggregationBuilder extends AggregationBuilder and it is possible but it'd be a mess. I found some other small cleanups I can make as a consolation prize. I'll get this rebased and in and open a PR for the consolation prize simplifications.

@nik9000 nik9000 force-pushed the named_xcontent_aggregations branch from 2b606c2 to 6c0f4c5 Compare January 9, 2017 18:59
@nik9000 nik9000 merged commit e3f77b4 into elastic:master Jan 9, 2017
@nik9000
Copy link
Member Author

nik9000 commented Jan 9, 2017 •
edited
Loading

master: e3f77b4
5.x: c392895

nik9000 added a commit that referenced this pull request Jan 9, 2017
@nik9000
Replace AggregatorParsers with namedObject (#22397)
c392895 
Removes `AggregatorParsers`, replacing all of its functionality with
`XContentParser#namedObject`.

This is the third bit of payoff from #22003, one less thing to pass
around the entire application.
@nik9000 nik9000 mentioned this pull request Jan 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@s1monw s1monw s1monw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>non-issue v5.2.0 v6.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nik9000 @s1monw