Skip to content

Conversation

@craigtaverner
Copy link
Contributor

@craigtaverner craigtaverner commented May 9, 2025

So, this was an odd one, two seemingly identical calculations can lead to different results under extreme conditions. Floating point arithmetic edge case. These two functions do not always agree:

  • double wrappedWidth = (180 - minPosX) - (-180 - maxNegX);
    • used in the test
    • results in exactly 360.0 when maxNegX was a very small negative number (about -1e-14 in the failing test)
  • double wrappedWidth = 360.0 + maxNegX - minPosX;
    • used in the production code
    • Results in 359.99999999... for the same case, which is more accurate

We now use the production formula in the test. The reason I wrote the original test formula was it was more explanatory than the production code. I did not expect a different result. Running with -Dtests.iters=10000 had 6 failures (0.06%, which is a very low failure rate). With this fix there were zero failures.

Fixes #123425

@craigtaverner craigtaverner added >test Issues or PRs that are addressing/adding tests :Analytics/Geo Indexing, search aggregations of geo points and shapes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels May 9, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@craigtaverner craigtaverner requested a review from GalLalouche May 9, 2025 16:20
@craigtaverner craigtaverner added v8.19.0 v8.18.2 v9.0.2 auto-backport Automatically create backport pull requests when merged labels May 9, 2025
@craigtaverner craigtaverner merged commit cb13913 into elastic:main May 10, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.19 Commit could not be cherrypicked due to conflicts
8.18 Commit could not be cherrypicked due to conflicts
9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 127982

@craigtaverner
Copy link
Contributor Author

Backports done manually in #128014

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 auto-backport Automatically create backport pull requests when merged Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.18.2 v8.19.0 v9.0.2 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] SpatialEnvelopeVisitorTests testVisitGeoPointsWrapping failing

3 participants