-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Add BinaryDocValuesField to replace BytesRef (ScriptDocValues) #79760
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Outdated
Show resolved
Hide resolved
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
Outdated
Show resolved
Hide resolved
@stu-elastic Thanks for solid feedback. Switched this over to byte[] and added utility to get these at utf8 to String. |
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 have a couple thoughts
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Outdated
Show resolved
Hide resolved
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Outdated
Show resolved
Hide resolved
@stu-elastic @rjernst This is ready for another round of review. |
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.
A couple thoughts, nothing blocking
...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/script/field/BinaryDocValuesField.java
Show resolved
Hide resolved
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpProcessor.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/script/field/DelegateDocValuesField.java
Outdated
Show resolved
Hide resolved
...toring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/ClusterAlertsUtilTests.java
Outdated
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 do the error messages separately.
However, please don't commit until the spotless changes are gone for files otherwise untouched by this PR.
...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongDocValuesField.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/script/field/BinaryDocValuesField.java
Outdated
Show resolved
Hide resolved
After discussion with @stu-elastic we're going to change the API a bit. We have decided to move the initial API to the following methods: getValue(default) -> get(default) This means users won't be able to get a List directly anymore, but instead can treat Field as an iterator. This decisions removes ambiguity over copied values and whether or not the List changes the Field that it was generate from. |
@stu-elastic Made the previously described changes. Would appreciate you looking them over one more time please. |
server/src/main/java/org/elasticsearch/script/field/DelegateDocValuesField.java
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.
Looks great.
Please ensure that EmptyField
still works for numeric fields.
...nless/src/yamlRestTest/resources/rest-api-spec/test/painless/60_script_doc_values_binary.yml
Show resolved
Hide resolved
...ang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/50_script_doc_values.yml
Show resolved
Hide resolved
Added getters for EmptyField along w/ tests. |
@stu-elastic @rjernst Thanks for the many reviews! |
This change creates the classes required for the scripting fields API to provide a binary field composed of doc values using BytesRef as the representation returned to the user as a value.