-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Update Painless Fields API design to use mapped types #78950
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
@elasticmachine run elasticsearch-ci/part-2 |
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Show resolved
Hide resolved
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Show resolved
Hide resolved
modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/100_terms_agg.yml
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/script/DocBasedScript.java
Outdated
Show resolved
Hide resolved
...pper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongField.java
Show resolved
Hide resolved
...unsigned-long/src/test/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldTests.java
Show resolved
Hide resolved
...ugin/mapper-unsigned-long/src/yamlRestTest/resources/rest-api-spec/test/50_script_values.yml
Show resolved
Hide resolved
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 can reuse a lot of the removed tests when we implement basic types in the new design, so let's save those (as requested above).
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Show resolved
Hide resolved
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.
Can you find a way to share the same doc values across ScriptDocValues
and and DocValuesField
? Having a penalty for using the fields API and doc
in the same script is pretty rough for adoption and is surprising behavior.
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.
lgtm
@stu-elastic @rjernst Thanks for the in depth reviews! I'm still working toward addressing all the feedback, and will commit once that's done. |
💔 Backport failed
You can use sqren/backport to manually backport by running |
This change updates the Painless fields API to use mapped types instead of Java types. For this initial release we only support unsigned long field. As we now intend to do conversions solely on the field classes themselves, converters and field values have been removed. Instead of using field values potentially as source fallback we will now rely on whatever class is passed into the script to support the field method. In the future, this method will need to check if a field exists on doc and if not, then do source fallback itself once that feature is supported. The path to retrieve a field is also different. Instead of using script doc values we now have our own way to retrieve the fields with a getScriptField method added to LeafDocLookup. This allows us less restrictive design space to generate the field type based the type in the leaf doc lookup; however, this will still require additional information that we need to plumb through for certain field types. Once we support all the field types this makes it easier to deprecate and remove script doc values.
…79201) This change updates the Painless fields API to use mapped types instead of Java types. For this initial release we only support unsigned long field. As we now intend to do conversions solely on the field classes themselves, converters and field values have been removed. Instead of using field values potentially as source fallback we will now rely on whatever class is passed into the script to support the field method. In the future, this method will need to check if a field exists on doc and if not, then do source fallback itself once that feature is supported. The path to retrieve a field is also different. Instead of using script doc values we now have our own way to retrieve the fields with a getScriptField method added to LeafDocLookup. This allows us less restrictive design space to generate the field type based the type in the leaf doc lookup; however, this will still require additional information that we need to plumb through for certain field types. Once we support all the field types this makes it easier to deprecate and remove script doc values.
This change updates the Painless fields API to use mapped types instead of Java types. For this initial release we only support unsigned long field.
As we now intend to do conversions solely on the field classes themselves, converters and field values have been removed. Instead of using field values potentially as source fallback we will now rely on whatever class is passed into the script to support the
field
method. In the future, this method will need to check if a field exists on doc and if not, then do source fallback itself once that feature is supported.The path to retrieve a field is also different. Instead of using script doc values we now have our own way to retrieve the fields with a
getScriptField
method added toLeafDocLookup
. This allows us less restrictive design space to generate the field type based the type in the leaf doc lookup; however, this will still require additional information that we need to plumb through for certain field types. Once we support all the field types this makes it easier to deprecate and remove script doc values.