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

Whitelist geo methods for Painless #40180

Closed
wants to merge 7 commits into from

Conversation

jdconrad
Copy link
Contributor

This change makes several classes relating to geo along with relevant methods and fields available as part of the standard whitelisting in Painless (across all contexts). This also adds the ability to add read-only non-static fields as part of a whitelist with an included test.

The following classes are added:

  • org.apache.lucene.geo.Rectangle
  • org.elasticsearch.common.unit.DistanceUnit
  • org.elasticsearch.common.unit.DistanceUnit$Distance
  • org.elasticsearch.common.geo.GeoPoint
  • org.elasticsearch.common.geo.GeoDistance
  • org.elasticsearch.common.geo.GeoUtils

Closes #24946

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.2.0 labels Mar 18, 2019
@jdconrad jdconrad requested a review from martijnvg March 18, 2019 20:27
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jdconrad
Copy link
Contributor Author

@nknize Hey Nick, would you please take a look at the methods I've added to the whitelist here and let me know if I'm missing anything that should be available or I've added things that don't make sense? Thanks in advance!

@jdconrad jdconrad added the WIP label Mar 18, 2019
@jdconrad
Copy link
Contributor Author

@martijnvg Please hold off on any final review for now as after speaking briefly with @rjernst I want to get some more feedback on what contexts should actually have these methods and if the method set is correct. Any thoughts on the contexts or method set though?

@martijnvg
Copy link
Member

@jdconrad The method set looks good to me. I think the following contexts should at least have these methods: filter, score, field and aggs.

@jdconrad jdconrad removed the WIP label Mar 25, 2019
@jdconrad
Copy link
Contributor Author

@martijnvg Hey Martijn, I've limited the contexts that geo can be used in, so this is ready for a final review now. Thank you!

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

@jdconrad This change looks good. Like we discussed today, it would be great to have some unit tests that test that these geo methods are actually available in these contexts.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This is a lot of random date stuff. Can we consolidate to what users actually need? For example, there are several distance methods on different distance classes, overlapping existing distance wrapper methods (eg on ScriptDocValues.GeoPoints).

@jdconrad
Copy link
Contributor Author

@rjernst Please tell me what you would include exactly

@rjernst
Copy link
Member

rjernst commented Mar 26, 2019

I think what we include should be agreed on with @nknize. IMO we should reduce to a single set of distance methods/representations.

@nknize
Copy link
Contributor

nknize commented Mar 26, 2019

@jdconrad I took a quick look. Everything whitelisted in this PR make complete sense to me. My inline comments list other classes we may want to consider adding. (e.g., the new ES geo library, lucene's Polygon and GeoUtils class). Am I understanding correctly that adding these classes and methods to the whitelist enable users to access those methods in the scripts? If so then I think adding those suggested classes would be very useful.

@jdconrad jdconrad added the WIP label Apr 1, 2019
@jdconrad
Copy link
Contributor Author

jdconrad commented Apr 1, 2019

@rjernst mentioned that some of the geo methods here might be up for refactoring soon, so I moved this back to a WIP while we come up with an appropriate plan for how to handle that situation

@jdconrad
Copy link
Contributor Author

jdconrad commented Apr 3, 2019

Had a brief meeting with @nknize and @rjernst. I'm going to close this PR for now as a lot of refactoring work is likely to occur in the near future on most of the geo classes whitelisted here. In the near term @nknize is going to add a method for bounding boxes after which we will whitelist the elasticsearch geo version of Rectangle in a new PR. GeoPoint and distance methods already exist in the standard whistelist and will be added to the API docs once the automated context API docs project is completed.

@jdconrad jdconrad closed this Apr 3, 2019
@nknize nknize mentioned this pull request Apr 4, 2019
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider additional java classes for whitelisting
6 participants