-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Use faster maths to project WGS84 to mercator #88231
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
Hi @iverase, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did have a question about skipping a test, but otherwise I think this looks good.
@@ -143,6 +143,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task -> | |||
task.skipTest("indices.freeze/10_basic/Test index options", "#70192 -- the freeze index API is removed from 8.0") | |||
task.skipTest("sql/sql/Paging through results", "scrolling through search hit queries no longer produces empty last page in 8.2") | |||
task.skipTest("service_accounts/10_basic/Test get service accounts", "new service accounts are added") | |||
task.skipTest("spatial/70_script_doc_values/diagonal length", "precision changed in 8.4.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we permanently skip this test? I see you changed the test result in the yaml file so do we need to skip this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are skipping the V7 compatibility test (yamlRestTestV7CompatTransform) . It seems this mode is running the test on the v7 branch against this cluster. That currently fails because the change in precision which is fine.
@@ -143,4 +143,4 @@ setup: | |||
script: | |||
source: "doc['geo_shape'].getMercatorHeight()" | |||
- match: { hits.hits.0.fields.width.0: 389.62170283915475 } | |||
- match: { hits.hits.0.fields.height.0: 333.37976840604097 } | |||
- match: { hits.hits.0.fields.height.0: 333.37976841442287 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we could assert on values with a precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not help here.
This commit changes the from strict maths to sloppy maths when we compute the projection from WGS84 to spherical mercator. Precision is more than enough and test shows a meaningful speed up when doing it for big geometries. the test performed locally shows a ~15% improvement.