[ES|QL] Text embedding inference operator#135062
Conversation
afoucret
added
>non-issue
v9.2.0
:Search Relevance/ES|QL
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
elasticsearchmachine
added
the
Team:Search Relevance
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/inference/InputTextReader.java
Show resolved
Hide resolved
afoucret
changed the title
carlosdelest
left a comment
carlosdelest
left a comment
There was a problem hiding this comment.
Looks good, thanks for dividing the original PR!
Some questions about testing, the rest looks 👍
...org/elasticsearch/xpack/esql/inference/textembedding/TextEmbeddingOperatorOutputBuilder.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/esql/inference/textembedding/TextEmbeddingOperatorOutputBuilder.java
Outdated
Show resolved
Hide resolved
| * This iterator reads text inputs from a {@link BytesRefBlock} and converts them into individual {@link InferenceAction.Request} instances | ||
| * of type {@link TaskType#TEXT_EMBEDDING}. | ||
| */ | ||
| public class TextEmbeddingOperatorRequestIterator implements BulkInferenceRequestIterator { |
There was a problem hiding this comment.
These classes may be included in their Operator class as inner classes - I don't think we're going to use them outside of the operators, right?
There was a problem hiding this comment.
I did not want the main classes to become to big.
I made them package-private instead of nesting them.
|
|
||
| public class TextEmbeddingOperatorOutputBuilderTests extends ComputeTestCase { | ||
|
|
||
| public void testBuildSmallOutputWithFloatEmbeddings() throws Exception { |
There was a problem hiding this comment.
For my understanding, what is expected to change from small to large, that we need to test both?
| public class TextEmbeddingOperatorRequestIteratorTests extends ComputeTestCase { | ||
|
|
||
| public void testIterateSmallInput() throws Exception { | ||
| assertIterate(between(1, 100)); |
There was a problem hiding this comment.
Same question, what is expected to change between small and large?
| final BytesRefBlock inputBlock = createMultiValueBlock(); | ||
|
|
||
| try (TextEmbeddingOperatorRequestIterator requestIterator = new TextEmbeddingOperatorRequestIterator(inputBlock, inferenceId)) { | ||
| // First position: multi-value concatenated with newlines |
There was a problem hiding this comment.
I'm not sure if we're testing what the comment says? I don't see a check for the multivalues 🤔
There was a problem hiding this comment.
Fixed the comment.
...rc/main/java/org/elasticsearch/xpack/esql/inference/textembedding/TextEmbeddingOperator.java
Show resolved
Hide resolved
3 participants
This PR implements an ES|QL inference operator that takes text input and return embeddings.
This work is part of #131079.