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

[feature/semantic-text] Simplify the integration of the field inference metadata in IndexMetadata #106743

Merged
merged 25 commits into from
Mar 28, 2024

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Mar 26, 2024

This change refactors the integration of the field inference metadata in IndexMetadata. Instead of partial diffs, the new class simply sends the entire object as diff if it has changed.
This PR also rename the fields and methods related to the inference fields consistently.
The inference phase (in the transport shard bulk action) is also changed so that inference is not called if:

  • The document contains a value for the inference input.
  • The document also contains a value for the inference results of that field (in the _inference map).

If the document contains no value for the inference input but an inference result for that field, it is marked as failed.

@jimczi jimczi added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Mar 26, 2024
@jimczi jimczi requested a review from carlosdelest March 26, 2024 09:45
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Mar 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

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

LGTM - I believe this is a simpler solution.

if (item.request() instanceof IndexRequest ir) {
indexRequest = ir;
} else if (item.request() instanceof UpdateRequest updateRequest) {
if (updateRequest.script() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, the update could be a script and not a doc. Thanks for fixing this

}
indexRequest = updateRequest.doc();
} else {
// ignore delete request
Copy link
Member

Choose a reason for hiding this comment

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

Good catch (x2)

@@ -119,6 +130,11 @@ public NestedObjectMapper getSubMappers() {
return subMappers;
}

@Override
public InferenceFieldMetadata getMetadata(Set<String> sourcePaths) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice - we use the same structure for retrieving metadata directly from the field type

index: test-sparse-index
body:
- '{"update": {"_id": "doc_1"}}'
- '{"doc":{"inference_field": "I am a test", "another_inference_field": "I am a teapot"}}'
Copy link
Member

Choose a reason for hiding this comment

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

"I am a teapot"

HTTP 418 FTW 🤣

@jimczi jimczi merged commit ef3abd9 into elastic:feature/semantic-text Mar 28, 2024
13 of 14 checks passed
@jimczi jimczi deleted the inference_metadata_diff branch March 28, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants