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

Fix geohash grid aggregation on multi-valued fields. #8513

Closed
wants to merge 2 commits into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Nov 17, 2014

This aggregation creates an anonymous fielddata instance that takes geo points
and turns them into a geo hash encoded as a long. A bug was introduced in 1.4
because of a fielddata refactoring: the fielddata instance tries to populate
an array with values without first making sure that it is large enough.

Close #8507

This aggregation creates an anonymous fielddata instance that takes geo points
and turns them into a geo hash encoded as a long. A bug was introduced in 1.4
because of a fielddata refactoring: the fielddata instance tries to populate
an array with values without first making sure that it is large enough.

Close elastic#8507
@jpountz jpountz added review v2.0.0-beta1 v1.5.0 v1.4.1 :Analytics/Aggregations Aggregations :Analytics/Geo Indexing, search aggregations of geo points and shapes >bug labels Nov 17, 2014
@@ -149,6 +149,7 @@ public void setDocument(int docId) {
geoValues = geoPointValues.geoPointValues();
geoValues.setDocument(docId);
count = geoValues.count();
grow();
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird API, where you must first set a protected member, then call a protected function. Perhaps it could be reworked to have count be private, and change grow() to resize(int)? I only see 2 cases where count is set, but grow() is not, and they are for 0 and 1, which I think are just special cases and can be skipped on entry to to resize?

@jpountz
Copy link
Contributor Author

jpountz commented Nov 17, 2014

@rjernst I pushed a new commit. Is it what you had in mind?

@rjernst
Copy link
Member

rjernst commented Nov 18, 2014

Yes, thanks. LGTM.

@jpountz jpountz removed the review label Nov 20, 2014
@jpountz jpountz closed this Nov 20, 2014
@clintongormley clintongormley changed the title Aggregations: Fix geohash grid aggregation on multi-valued fields. Fix geohash grid aggregation on multi-valued fields. Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations :Analytics/Geo Indexing, search aggregations of geo points and shapes >bug v1.4.1 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants