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

[inference_metadata_fields] Fix handling of explicit null values for semantic text fields #118947

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Dec 18, 2024

Previously, setting a field explicitly to null in an update request did not work correctly with semantic text fields. This change resolves the issue by adding an explicit null entry to the _inference_fields metadata when such cases occur.

The explicit null value ensures that any prior inference results are overwritten during the merge of the partial update with the latest document version.

Previously, setting a field explicitly to null in an update request did not work correctly with semantic text fields.
This change resolves the issue by adding an explicit null entry to the `_inference_fields` metadata when such cases occur.

The explicit null value ensures that any prior inference results are overwritten during the merge of the partial update with the latest document version.
@jimczi jimczi added >non-issue :SearchOrg/Relevance Label for the Search (solution/org) Relevance team v9.0.0 labels Dec 18, 2024
@jimczi jimczi requested a review from Mikep86 December 18, 2024 12:51
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-eng (Team:SearchOrg)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-relevance (Team:Search - Relevance)

@jimczi
Copy link
Contributor Author

jimczi commented Dec 18, 2024

@Mikep86 I'll wait for #118841 to be merged before adding YAML tests for this.
The logic is straightforward, so I opened the PR early since you mentioned that a refactoring was needed to fix it, but I don't think it's necessary.

Copy link
Contributor

@Mikep86 Mikep86 left a comment

Choose a reason for hiding this comment

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

Thanks for writing up this approach. I agree that we can fix this issue with less refactoring than I originally thought by creating a response accumulator slot when a source field value is explicitly set to null.

However, I think the approach needs some adjustment to handle semantic text fields with multiple source fields. Based on my reading, I think we can handle this case by adding a FieldInferenceResponse with an "empty" ChunkedInference result (i.e. no chunks) when we encounter an explicit null value. This would result in applyInferenceResponses generating an empty chunk list for the applicable source field.

@Mikep86
Copy link
Contributor

Mikep86 commented Dec 19, 2024

I created #119145 to follow up on this issue

@jimczi jimczi closed this Dec 20, 2024
@jimczi jimczi deleted the inference_metadata_fields_explicit_null branch December 20, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :SearchOrg/Relevance Label for the Search (solution/org) Relevance team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants