Block hybrid query when no search pipeline is configured - #1924
Block hybrid query when no search pipeline is configured#1924AdityaWaskar wants to merge 2 commits into
Conversation
Signed-off-by: AdityaWaskar <adityawaskar05@gmail.com>
PR Reviewer Guide 🔍(Review updated until commit 023078f)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 023078f
Previous suggestionsSuggestions up to commit 4ea01d6
|
Signed-off-by: AdityaWaskar <adityawaskar05@gmail.com>
|
One design decision worth confirming: for requests spanning multiple indices (or an alias/wildcard), this PR requires every matched index to have a default search pipeline configured (or the request itself to set one) before a hybrid query is allowed to proceed — checked via I wasn't sure whether that's the right semantics, or whether the default-search-pipeline fallback should only apply to single-index requests (mirroring how I believe |
|
Persistent review updated to latest commit 023078f |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1924 +/- ##
============================================
- Coverage 83.45% 83.41% -0.04%
- Complexity 3884 3889 +5
============================================
Files 291 291
Lines 13819 13832 +13
Branches 2294 2299 +5
============================================
+ Hits 11532 11538 +6
- Misses 1454 1456 +2
- Partials 833 838 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| * @param searchRequest the search request to check | ||
| * @return true if a non-noop pipeline can be resolved for this request | ||
| */ | ||
| private boolean hasResolvableSearchPipeline(SearchRequest searchRequest) { |
There was a problem hiding this comment.
I think we should inspect the pipeline to confirm the normalization and combination processors are configured otherwise a search pipeline without those processors still cannot return the right hybrid query result.
There was a problem hiding this comment.
+1 here. Please add a validation to check the normalization and combination processors attached and an associated test
martin-gaievski
left a comment
martin-gaievski
left a comment
There was a problem hiding this comment.
Overall that's the right direction but you need to address comments, in addition to that add a single integ tests for failure path
| * @param searchRequest the search request to check | ||
| * @return true if a non-noop pipeline can be resolved for this request | ||
| */ | ||
| private boolean hasResolvableSearchPipeline(SearchRequest searchRequest) { |
There was a problem hiding this comment.
this logic diverges from SearchPipelineService. Core method resolvePipeline checks source().searchPipelineSource() first, before the named param.
When Core resolves an inline-body ad-hoc pipeline, request's searchPipelineSource map is not drained — meaning it is still intact when this ActionFilter runs (the filter runs before resolvePipeline). An inline-object body sets searchPipelineSource (SearchSourceBuilder.java) and leaves searchRequest.pipeline() null (RestSearchAction only populates pipeline() from the ?search_pipeline= param or the string form of source().pipeline()). So isConfiguredPipeline(null) is false, no index default exists, and the request is wrongly rejected with a 400, even core would build and run the inline normalization pipeline correctly.
Example of failing request, where index has no default pipeline, no ?search_pipeline= param:
POST /my-index/_search
{
"search_pipeline": {
"phase_results_processors": [
{ "normalization-processor": {
"normalization": { "technique": "min_max" },
"combination": { "technique": "arithmetic_mean" } } }
]
},
"query": { "hybrid": { "queries": [ {"match":{"text":"hello"}}, {"term":{"text":"place"}} ] } }
}You can fail fast on the inline body before the named/default checks, mirroring core branch.
| } | ||
|
|
||
| List<IndexMetadata> indexMetadataList = NeuralSearchClusterUtil.instance().getIndexMetadataList(searchRequest); | ||
| return indexMetadataList.isEmpty() == false && indexMetadataList.stream().allMatch(this::hasDefaultSearchPipeline); |
There was a problem hiding this comment.
this logic is different in core, and fails in multiple scenarios:
- it collapses to
_none→NO_OP_PIPELINE→ no normalization runs → exact unnormalized/internal-format response we're trying to address. Intended behavior is to fail fast - multi-index mixed defaults → false rejection (over-strict vs. core). with your change we're skipping indices that have no explicit default rather than treating them as
_none.
I suggest you don't allMatch over raw .get() values.
Most robust approach is to delegate to core so the filter cannot diverge.
Alternatively fold per-index defaults with core's exact semantics — use .exists() to skip indices with no default, take the first default, collapse to _none if a later index has a different pipeline, then accept only if the folded id is a configured (non-_none) pipeline.
| return; | ||
| } | ||
| if (hasResolvableSearchPipeline(searchRequest) == false) { | ||
| listener.onFailure(new IllegalArgumentException(HybridQueryUtil.HYBRID_QUERY_REQUIRES_SEARCH_PIPELINE_MESSAGE)); |
There was a problem hiding this comment.
provided error message is over promising - we are checking for a simple search pipeline presence, not normalization related processors. Either add check for processor or relax the message and drop mentions of processors (which is much easier fix I believe)
4 participants
Description
Hybrid query currently executes even when no search pipeline is configured for the request, silently returning results in an internal, unnormalized format instead of failing clearly. This adds a coordinator-level check (in
HybridQuerySearchRequestFilter) that rejects hybrid query requests when no search pipeline can be resolved — neither via the request (inline orsearch_pipelineparam) nor as every target index's default search pipeline.Related Issues
Resolves #1922