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 max-int limit for number of points reduced in geo_centroid #56370

Merged
merged 4 commits into from May 7, 2020

Conversation

talevy
Copy link
Contributor

@talevy talevy commented May 7, 2020

A bug in InternalGeoCentroid#reduce existed that summed up
the aggregation's long-valued counts into a local integer variable.
Since it is definitely possible to reduce more than Integer.MAX points,
this change simply updates that variable to be a long-valued number.

Closes #55992.

A bug in InternalGeoCentroid#reduce existed that summed up
the aggregation's long-valued counts into a local integer variable.
Since it is definitely possible to reduce more than Integer.MAX points,
this change simply updates that variable to be a long-valued number.

Closes elastic#55992.
@talevy talevy added :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.0.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.8.1 v7.9.0 labels May 7, 2020
@talevy talevy requested review from nknize and iverase May 7, 2020 17:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

@@ -115,7 +115,7 @@ public long count() {
public InternalGeoCentroid reduce(List<InternalAggregation> aggregations, ReduceContext reduceContext) {
double lonSum = Double.NaN;
double latSum = Double.NaN;
int totalCount = 0;
long totalCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Huh! The count member variable was always a long!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup!

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh!

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Much ❤️

@talevy
Copy link
Contributor Author

talevy commented May 7, 2020

thanks Nik!

@talevy talevy merged commit 10bbf05 into elastic:master May 7, 2020
@talevy talevy deleted the fix-55992 branch May 7, 2020 21:29
talevy added a commit that referenced this pull request May 7, 2020
A bug in InternalGeoCentroid#reduce existed that summed up
the aggregation's long-valued counts into a local integer variable.
Since it is definitely possible to reduce more than Integer.MAX points,
this change simply updates that variable to be a long-valued number.

Closes #55992.
talevy added a commit that referenced this pull request May 7, 2020
A bug in InternalGeoCentroid#reduce existed that summed up
the aggregation's long-valued counts into a local integer variable.
Since it is definitely possible to reduce more than Integer.MAX points,
this change simply updates that variable to be a long-valued number.

Closes #55992.
@Mpdreamz Mpdreamz added the >bug label Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.8.1 v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Int 32 overflow on geo centroid aggregation
6 participants