Add BinaryDocValuesField to replace BytesRef (ScriptDocValues) #79760
Conversation
jdconrad
added
>enhancement
:Core/Infra/Scripting
|
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. |
...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. |
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
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
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
Show resolved
Hide resolved
stu-elastic
left a comment
stu-elastic
left a comment
There was a problem hiding this comment.
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
stu-elastic
left a comment
stu-elastic
left a comment
There was a problem hiding this comment.
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! |
4 participants
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.