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.

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

Run pipeline aggregations for empty buckets added in the Range Aggregation #15519

Merged
merged 1 commit into from Dec 18, 2015

Conversation

Projects
None yet
2 participants
@colings86
Copy link
Member

commented Dec 17, 2015

Closes #15471

@jpountz

View changes

modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/BucketSelectorTests.java Outdated
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.lessThan;
import static org.hamcrest.Matchers.notNullValue;

This comment has been minimized.

Copy link
@jpountz

jpountz Dec 18, 2015

Contributor

hmm, why did imports move that much?

This comment has been minimized.

Copy link
@colings86

colings86 Dec 18, 2015

Author Member

I think something might have changed in the import ordering in the IDE. Not sure if that was a project setting changed in master or if it's just my Eclipse, I will check.

@jpountz

View changes

modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/BucketSelectorTests.java Outdated
SearchResponse response = client().prepareSearch("idx_with_gaps")
.addAggregation(histogram("histo").field(FIELD_1_NAME).interval(1)
.subAggregation(range("range").field(FIELD_2_NAME).addRange(1, 2).addRange(2, 3).addRange(3, 4)
.subAggregation(having("bucket_selector").setBucketsPaths("_count").script(new Script("_value0 > 0")))))

This comment has been minimized.

Copy link
@jpountz

jpountz Dec 18, 2015

Contributor

is it possible to write this test in a way that it either does not require a script, or uses a mock script?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2015

The fix looks good to me, it's surprisingly simple! I'm more concerned about the test however: can we make it not use an actual script?

@colings86 colings86 force-pushed the colings86:fix/15471 branch Dec 18, 2015

@colings86

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2015

@jpountz I pushed a new commit which changes the test so it doesn't use scripts

@colings86 colings86 force-pushed the colings86:fix/15471 branch Dec 18, 2015

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2015

LGTM

@colings86 colings86 force-pushed the colings86:fix/15471 branch to 15588a4 Dec 18, 2015

@colings86 colings86 merged commit 15588a4 into elastic:master Dec 18, 2015

1 check passed

CLA Commit author has signed the CLA
Details

@colings86 colings86 added the v2.2.0 label Dec 18, 2015

@clintongormley clintongormley changed the title Aggregations: Run pipeline aggregations for empty buckets added in the Range Aggregation Run pipeline aggregations for empty buckets added in the Range Aggregation Jan 10, 2016

@colings86 colings86 deleted the colings86:fix/15471 branch Jan 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.