Cleanup: Remove obsolete TODO from SparseVectorQueryBuilder

[mridula-s109]

Summary

Small cleanup to bring SparseVectorQueryBuilder in line with current inference APIs and the pattern used by SemanticQueryBuilder.

Changes

• Replace CoordinatedInferenceAction.Request with InferenceAction.Request (parity with SemanticQueryBuilder).
• Remove outdated TODO about moving this to xpack core (it’s already in core).
• Refactor response handling to a validateAndExtractTextExpansionResults(...) helper for clearer errors and consistency.

[mridula-s109]

Hi @kderusso 👋

This draft PR addresses the SparseVectorQueryBuilder refactoring.

Highlights:

• Standardized on InferenceAction.Request (no need for CoordinatedInferenceAction.Request)
• Clean separation from ML coordination logic
• Minimal, mostly mechanical refactoring — tests all pass

Would love your thoughts, especially on whether the InferenceAction.Request standardization and overall approach match the intended architectural direction. 🙏

[kderusso]

Hey @mridula-s109 so sorry about the confusion here, the Jira ticket you were assigned said that Sparse Vector was still in ml and we didn't verify that it had already been moved to core during planning 😅

I think the cleanup refactoring that you've done here makes sense for the most part, with some caveats:

• I would like to understand the difference between a CoordinatedInferenceAction and an InferenceAction. By changing this, what is actually changing under the hood and what side effects are there? Knowing that someone has done this research is important for peace of mind
• The bulk of the work for this type of refactor lies in test failures - and there are a lot of these in your PR.

It would be worth thinking about whether we need this PR, or since it's already in core if we just try to do a refactoring to server.

[mridula-s109]

Hey @mridula-s109 so sorry about the confusion here, the Jira ticket you were assigned said that Sparse Vector was still in ml and we didn't verify that it had already been moved to core during planning 😅

I think the cleanup refactoring that you've done here makes sense for the most part, with some caveats:

• I would like to understand the difference between a CoordinatedInferenceAction and an InferenceAction. By changing this, what is actually changing under the hood and what side effects are there? Knowing that someone has done this research is important for peace of mind
• The bulk of the work for this type of refactor lies in test failures - and there are a lot of these in your PR.

It would be worth thinking about whether we need this PR, or since it's already in core if we just try to do a refactoring to server.

Hey @kderusso, thanks for clarifying!Since sparse vector is already in core, would you be okay with me closing this PR and also resolving the Jira issue?

[kderusso]

Hey @kderusso, thanks for clarifying!Since sparse vector is already in core, would you be okay with me closing this PR and also resolving the Jira issue?

Can you please open a smaller PR to clear out the TODOs to avoid confusion down the line?

[kderusso]

Also, did you time box looking into moving this to server? How bad would it be?

[kderusso]

I noticed that sparse vector yaml tests are still in ML, if we're keeping this PR we should probably move them (unless there's a compelling "this will break everything" reason to keep them)

[mridula-s109]

Hey @mridula-s109 so sorry about the confusion here, the Jira ticket you were assigned said that Sparse Vector was still in ml and we didn't verify that it had already been moved to core during planning 😅

I think the cleanup refactoring that you've done here makes sense for the most part, with some caveats:

• I would like to understand the difference between a CoordinatedInferenceAction and an InferenceAction. By changing this, what is actually changing under the hood and what side effects are there? Knowing that someone has done this research is important for peace of mind
• The bulk of the work for this type of refactor lies in test failures - and there are a lot of these in your PR.

It would be worth thinking about whether we need this PR, or since it's already in core if we just try to do a refactoring to server.

Thanks for the thoughtful feedback @kderusso and @ioanatia 🙏

To clarify the CoordinatedInferenceAction vs InferenceAction part:

• CoordinatedInferenceAction is used when we need to coordinate inference requests across clusters (CCS mode, reduce roundtrips=false, etc.). It wraps results into a “coordination format.”
• InferenceAction is the simpler path for local inference requests within a single cluster. It doesn’t do the extra cross-cluster handling.

Since SparseVector queries don’t currently need cross-cluster coordination, switching to InferenceAction.Request aligns with what we already do in SemanticQueryBuilder. Functionally, this shouldn’t change behaviour for local queries, but it does reduce unnecessary complexity.

On the scope of the refactor:

• You’re right that SparseVectorQueryBuilder is already in core, so the big move work is unnecessary.
• I’ve now aligned it with the SemanticQueryBuilder pattern (as suggested by Ioana) and cleaned up the TODO to avoid future confusion.
• The remaining open question is whether we should take the next step of moving this fully into server (like knn). I haven’t deeply time-boxed that yet my sense is it’s a larger lift and may be best tracked separately.

[mridula-s109]

Hey @mridula-s109 so sorry about the confusion here, the Jira ticket you were assigned said that Sparse Vector was still in ml and we didn't verify that it had already been moved to core during planning 😅
I think the cleanup refactoring that you've done here makes sense for the most part, with some caveats:

• I would like to understand the difference between a CoordinatedInferenceAction and an InferenceAction. By changing this, what is actually changing under the hood and what side effects are there? Knowing that someone has done this research is important for peace of mind
• The bulk of the work for this type of refactor lies in test failures - and there are a lot of these in your PR.

It would be worth thinking about whether we need this PR, or since it's already in core if we just try to do a refactoring to server.

Thanks for the thoughtful feedback @kderusso and @ioanatia 🙏

To clarify the CoordinatedInferenceAction vs InferenceAction part:

• CoordinatedInferenceAction is used when we need to coordinate inference requests across clusters (CCS mode, reduce roundtrips=false, etc.). It wraps results into a “coordination format.”
• InferenceAction is the simpler path for local inference requests within a single cluster. It doesn’t do the extra cross-cluster handling.

Since SparseVector queries don’t currently need cross-cluster coordination, switching to InferenceAction.Request aligns with what we already do in SemanticQueryBuilder. Functionally, this shouldn’t change behaviour for local queries, but it does reduce unnecessary complexity.

On the scope of the refactor:

• You’re right that SparseVectorQueryBuilder is already in core, so the big move work is unnecessary.
• I’ve now aligned it with the SemanticQueryBuilder pattern (as suggested by Ioana) and cleaned up the TODO to avoid future confusion.
• The remaining open question is whether we should take the next step of moving this fully into server (like knn). I haven’t deeply time-boxed that yet my sense is it’s a larger lift and may be best tracked separately.

I tested everything locally with ./gradlew :x-pack:plugin:inference:check and all the tests passed. I’ll also double-check any failures once CI finishes running.

[kderusso]

Since SparseVector queries don’t currently need cross-cluster coordination,

Is that true, or would that break CCS for sparse vector search?

[ioanatia]

Since SparseVector queries don’t currently need cross-cluster coordination,

Is that true, or would that break CCS for sparse vector search?

This does not break CCS support. The CCS support that we recently added for SparseVectorQueryBuilder kicks in only when we query semantic_text fields with sparse_vector. In that case, the query interceptor will be responsible for getting the inference results and rewriting the sparse_vector query. So the code path that Mridula touches here will not even be reached.

Taking a closer look at CoordinatedInferenceAction its purpose is to be a unified, internal API that can handle multiple types of tasks and that is able to call the inference API or internal ML models (using the infer model internal API).
If used with inference endpoints that refer to Elasticsearch models hosted on the ML node, it will call the infer model API directly.
The same InferModelAction is called by the inference service, when used with an inference endpoint that points to an ML model that is being run by ML Elasticsearch nodes.
I don't really see how any of these would break CCS.

This change actually breaks something else, in a more subtle way.
Right now on main, for the sparse_vector query, the inference_id can point to both an Inference Endpoint and a model ID.

On this branch, if inference_id points to a model ID, the query will break.
However, I don't think we ever intended to support model_id, the inference_id option was always documented as an inference API endpoint.

[elasticsearchmachine]

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

[elasticsearchmachine]

Hi @mridula-s109, I've created a changelog YAML for you.

[mridula-s109]

As its already removed to core, just removed the redundant todo @kderusso . We can evaluate moving to server later when necessary.

[kderusso]