Repository navigation
Fix #2597: refactor the findAndRerank command path into a planner and read legs - #2637
Open
Hazel-Datastax wants to merge 23 commits into
Open
Hazel-Datastax wants to merge 23 commits into
Hazel-Datastax wants to merge 23 commits into
Conversation
Contributor
📈 Unit Test Coverage Delta vs Main Branch
|
Contributor
Unit Test Coverage Report
|
Contributor
📉 Integration Test Coverage Delta vs Main Branch (dse69-it)
|
Contributor
Integration Test Coverage Report (dse69-it)
|
Contributor
📉 Integration Test Coverage Delta vs Main Branch (hcd-it)
|
Contributor
Integration Test Coverage Report (hcd-it)
|
Add RERANK_MODEL, VECTORIZE_LEXICAL_RERANK_COLLECTION_SCHEMA_OBJECT and the rerankingProvidersConfig() / rerankingModelConfig() factories, so the findAndRerank tests stop building their own copies of the schema and the reranking providers config.
MISSING_HYBRID_SORT_VALUE and UNSUPPORTED_FIND_AND_RERANK_SORT are errors in the sort clause, so they move from RequestException.Code to SortException.Code, and to scope SORT in errors.yaml. UNSUPPORTED_FIND_AND_RERANK_SORT is only raised for a missing, null or empty sort, so its title and first line now say the command did not include a $hybrid sort, instead of "does not support this sort yet".
- Rename LegMode to SortMode, it is the sort the user wrote, not the legs that run. Keep only HYBRID and FILTER, the modes the parser produces; the single key sorts get their modes with GH #2601 / #2602. - FindAndRerankSort checks that commandFeatures is set and that a FILTER sort has no sort values, and documents what it is. Remove the 4 arg constructor that only tests used. - FindAndRerankPlanner.plan() takes only the sort and reads the mode and explicitLexical from it, so a caller cannot pass values that disagree. - The plan() javadoc lists the error order and why the rerank query and passage errors come before the sort errors. - Keep the sanity check for an explicit $lexical without a lexical index where the legs are built, with the check shared with checkSortSupported(). - Name the hybridLimits fields from the DocumentConstants the deserializer reads, and comment the assumptions and the follow-ups (GH #2603, #2616). - FindAndRerankPlannerTest builds HYBRID sorts with explicitLexical at the call site, uses the TestConstants schema objects and providers config, keeps the fail* tests with the area they cover, and types the expected error as ErrorCode<T>.
- RerankLeg extends Recordable and FindAndRerankPlan records its legs, so they read well in logs and traces. VectorLeg records the vector length, not the values. - The legs and RerankingQuery.create() are package-private, the builder only sees RerankLeg. readLimit() and needsVectorize() leave the interface; InnerRead.deferrables() gives the builder the deferred vectorize, if any. - InnerRead.create() null checks the sort clause. The lexical and vector legs use SortClause.immutable(), only the vectorize leg needs a mutable clause. The RerankLeg javadoc no longer calls the legs immutable. - The builder pairs each leg with its deferred read once, in a small record, instead of matching two lists by index in two methods. - Remove TaskAndDeferrables, the builder was its last user. - Move the leg and plan tests out of FindAndRerankPlannerTest into RerankLegTests and FindAndRerankPlanTests. They now cover every includeScores / includeSortVector combination for each leg, compare whole sort expressions, and check what the vectorize leg embeds.
- FindAndRerankOperationBuilderBehaviorTest: success rows also check the reads the operation runs (rank source and limit, in order), the expected error is an ErrorCode<?> in a Failure record, Jackson reads the single quoted JSON, the collections use the planner test names, the fixtures come from TestConstants, and the javadoc and row comments say what each changed row pins rather than its history. - FindAndRerankOperationBuilderTest: uses the shared schema and providers config, checks the embedding task completes the deferred vectorize of the read that waits for it, and drops assertMissingRerankOn(). - Share the task tree helpers as FindAndRerankOperationAssertions, with ReadTaskSnapshot and assert* names. - RerankingQueryTests is one parameterized test over the input rows.
Rename SortMode back to LegMode, and put VECTORIZE, VECTOR and LEXICAL back with a TODO for GH #2602: the parser does not produce them until findAndRerank supports single leg sorts. The planner rejects them with UNSUPPORTED_FIND_AND_RERANK_SORT, like FILTER, and a planner test covers the three modes.
Go back to main's NO_ARG_SORT constant for a request without a sort. The sort's CommandFeatures are only read, so a shared instance is fine; that CommandFeatures.EMPTY is mutable is a separate issue. CommandFeatures.contains() has no callers now that the planner reads explicitLexical from the sort.
Main's builder gave both inner reads includeScores as includeSimilarity. The BM25 read does not use it, so nothing changes for users, but the refactor should not change what the inner reads get.
Use the "The command attempted to ..." form for MISSING_HYBRID_SORT_VALUE and UNSUPPORTED_FIND_AND_RERANK_SORT. The second one is only raised for a missing, null or empty sort today, so it says that.
Undo making create() package-private, and go back to the four plain RerankingQueryTests rather than one parameterized test that switches on a null expected query.
Keep the two inline notes from before: the steps run in the order of the errors, and the sort checks come after the rerank query and passage checks.
- The hybridLimits asserts check "Command field '<field>' value <n> not valid". The old asserts looked for "0" on its own, which the bounds text "between 1 and 100" always contains. - The builder tests name the expected exception class, so moving an error code to another exception class fails the test. - Rename assertReadTasks() to readTaskSnapshots(), it returns the reads. - Test that VectorLeg equals and hashCode compare the vector values.
The sort parser copied {"$hybrid": "text"} into both vectorizeSort and
lexicalSort, so the sort needed an explicitLexical flag to tell it apart
from a $hybrid object with the same text. The parser now keeps the text in
hybridText, and the planner decides its legs from the collection: a
vectorize leg, and a lexical leg when there is a lexical index.
Should not change behaviour. The MCP tool schema for the sort now has
hybridText in place of explicitLexical.
A $hybrid sort must always have a vector or vectorize leg. A $hybrid object
with a $lexical value and no $vectorize or $vector value used to run only
the lexical read, it now fails with the new SORT error
MISSING_VECTOR_OR_VECTORIZE_IN_HYBRID_SORT. The check is one of the sort
support checks: it comes after the lexical index check, and before the
hybridLimits, rerank service, rerankQuery, rerankOn and projection checks.
A sort with no value at all still fails with MISSING_HYBRID_SORT_VALUE.
Reading with only $lexical will be the single leg sort {"$lexical": ...},
GH #2602.
The API returns HTTP 200 with UNEXPECTED_SERVER_ERROR for an unexpected exception such as the NoSuchElementException from orElseThrow().
…rror A $hybrid sort that is text only runs the lexical search when the collection has a lexical index.
{"$hybrid": null} is rejected by the sort parser, one of the requests in
#2577 had no test for it. The TODO marks the UNSUPPORTED_FIND_AND_RERANK_SORT
check that changes when the planner supports LegMode.FILTER.
The rerank query and the field to rerank on came from the first leg that had a default. That only worked because the vectorize leg was the only leg with defaults: the lexical leg is always first, so a lexical default would have silently replaced the $vectorize defaults of a $hybrid sort. The planner now decides the defaults from the leg mode. A $hybrid sort takes them from its $vectorize text, as before the planner, and the order of the legs only decides the order of the read tasks. RerankLeg no longer has defaultRerankQuery() and defaultPassageField(). Should not change behaviour.
Hazel-Datastax
changed the title
Hazel-Datastax
marked this pull request as ready for review
October 14, 2026 18:35
erichare
reviewed
Oct 14, 2026
| int limit) | ||
| implements Recordable { | ||
|
|
||
| public FindAndRerankPlan { |
Contributor
There was a problem hiding this comment.
Can we also check that limit is positive in this constructor, so callers cannot create an invalid plan?
erichare
reviewed
Oct 14, 2026
| /** | ||
| * A leg that reads the documents most similar to a vector the user provided. | ||
| * | ||
| * <p>NOTE: the vector array is not copied, it is the vector from the parsed sort and nothing |
Contributor
There was a problem hiding this comment.
Can we copy the vector instead of sharing the array? Otherwise, changing the array also changes this record’s data and hash code.
erichare
reviewed
Oct 14, 2026
| * <p>TODO: GH #2602 - the parser does not produce {@link #VECTORIZE}, {@link #VECTOR} or {@link | ||
| * #LEXICAL} yet, they will be implemented when findAndRerank supports single leg sorts. | ||
| */ | ||
| public enum LegMode { |
Contributor
There was a problem hiding this comment.
Could we add VECTORIZE, VECTOR, and LEXICAL with #2602? The parser cannot produce them yet, so they currently only add rejection branches and tests.
Contributor
There was a problem hiding this comment.
Change to be called RerankLegMode - with two values, SINGLE_LEG and MULTI_LEG
multi is when $hybrid is used in the sort clause, otherwise it is single.
Add a new RerankLegType enum, use that when you have a leg. Make the values VECTOR_LEG, LEXICAL_LEG, FILTER_LEG
erichare
reviewed
Oct 14, 2026
| // these come after the rerank query and passage checks, so a request with no valued sort (e.g. | ||
| // {} or {"$hybrid": ""}) first gets any rerankQuery / rerankOn error | ||
| // (MISSING_RERANK_QUERY_TEXT, MISSING_RERANK_ON, or an invalid rerankOn path) | ||
| // TODO(Hazel to Aaron): This order is the same as the error order before the planner. I think |
Contributor
There was a problem hiding this comment.
I'd preserve the existing error order for this refactor and discuss changing it separately. Can we replace this TODO with a comment explaining that compatibility decision?
erichare
reviewed
Oct 14, 2026
erichare
left a comment
erichare
left a comment
Contributor
There was a problem hiding this comment.
The planner/builder split makes sense to me. I left a few comments above. One other thought: could we use less reflection in the builder tests? Renaming a private field could break these even when the behavior hasn’t changed. Maybe we can check the plan directly for some of those cases.
erichare
self-requested a review
October 14, 2026 19:23
erichare
approved these changes
Oct 14, 2026
This was referenced Oct 15, 2026
amorton
requested changes
Oct 19, 2026
amorton
left a comment
amorton
left a comment
Contributor
There was a problem hiding this comment.
-1 I like the direction but:
- the AI is still writing low quality code and code comments
- I am nervious with changing how the reranking operation runs (the work with deferred results) , that is not needed just to start the idea of a plan.
- the resolver should make a plan by passing the command to a planner, and then pass the plan to the builder. This will make testing a lot easier.
- i have not read any of the tests
| * @param hybridText The trimmed text, <code>null</code> when the text was blank. | ||
| * @param commandFeatures The features used by the sort clause, only used for metrics. | ||
| */ | ||
| public static FindAndRerankSort fromHybridText( |
Contributor
There was a problem hiding this comment.
-1 for ths and fromHybridObject - the deserialiser is responsible for creating instances, it should do that via the ctor, and there should not be factory functions on this class.
| public record FindAndRerankSort( | ||
| String vectorizeSort, String lexicalSort, float[] vectorSort, CommandFeatures commandFeatures) | ||
| LegMode legMode, | ||
| String hybridText, |
| normalizedText, normalizedText, null, CommandFeatures.of(CommandFeature.HYBRID)); | ||
| } | ||
| case TextNode textNode -> | ||
| FindAndRerankSort.fromHybridText( |
Contributor
There was a problem hiding this comment.
-1 remove the factory added to the Sort clause, the code adds nothing.
|
|
||
| var sortMatch = MATCH_SORT_FIELDS.matchAndThrow(hybridObject, jsonParser, ERROR_CONTEXT); | ||
| // NOTE: commandFeatures are only for metrics, the FindAndRerankPlanner decides what to run | ||
| // from the legMode and the sort values |
Contributor
There was a problem hiding this comment.
-1 removet his comment, it is not needed and should not talk about how the rest of the code uses commandFeatures
| }; | ||
|
|
||
| return new FindAndRerankSort(vectorizeText, lexicalText, vector, commandFeatures); | ||
| return FindAndRerankSort.fromHybridObject(vectorizeText, lexicalText, vector, commandFeatures); |
Contributor
There was a problem hiding this comment.
-1 the factory added is not needed
| * @param query The default rerank query, <code>null</code> when there is none. | ||
| * @param passageField The default field to rerank on, <code>null</code> when there is none. | ||
| */ | ||
| private record RerankDefaults(String query, String passageField) { |
Contributor
There was a problem hiding this comment.
this seems like a bad class name, Defaults ??? where are they ?
| * @param vectorizeDefinition The vectorize service of the collection. | ||
| * @param readLimit The maximum number of documents to read. | ||
| */ | ||
| record VectorizeLeg( |
Contributor
There was a problem hiding this comment.
there should only be a Vector leg , it is not different depending on the sort
| * DeferredVectorize}, are only created when {@link #buildInnerRead(FilterDefinition, boolean, | ||
| * boolean)} is called. | ||
| * | ||
| * <p>NOTE: this interface is sealed so that a switch over the legs must handle every kind of leg. |
Contributor
There was a problem hiding this comment.
this really does not need to be a in a comment
| * <p>TODO: GH #2603 - the leg that reads with only the filter must be added to this package and to | ||
| * the permits clause. | ||
| */ | ||
| public sealed interface RerankLeg extends Recordable permits LexicalLeg, VectorLeg, VectorizeLeg { |
Contributor
There was a problem hiding this comment.
-1 on this.
- This is part of the plan, but it also has business logic on how to make the operations ? If it's the plan it should be as POJO and just data.
- we do not need to refactor how the commands works just to get the plan in
| * only turns the {@link FindAndRerankPlan} into tasks: one read task per leg, an embedding task for | ||
| * the legs that need vectorizing, and one reranking task over the results of all the reads. | ||
| */ | ||
| class FindAndRerankOperationBuilder { |
Contributor
There was a problem hiding this comment.
-1 - I want to discuss if we can bring this PR down to just being the plan, not changing the way the tasks are put together.
Can we make the plan, and then have the builder follow the plan without changing how the commands run. There is a lot of risk in changing that code all at once
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does:
The findAndRerank builder decided and built in one class, with two fixed read slots. So a sort without a vector source failed with
UNEXPECTED_SERVER_ERROR, and the single-leg sorts that we need to support had no place to go. This PR splits the command path into parse -> plan -> build: the planner picks the reads (legs), and the builder only makes the tasks. Valid requests work as before. This is the base for #2602 (single-leg sorts) and #2603 (filter leg).Not in this PR: single-leg sorts (#2602), the filter leg (#2603), rejecting
$vectorizewith$vector(#2616), and an MCP tool schema for the sort that shows the request JSON, not the record fields.Tests: unit tests for the parser, planner, legs and plan, and
FindAndRerankOperationBuilderBehaviorTest, which pins the result of 75 request shapes.Which issue(s) this PR fixes:
Fixes #2597
Fixes #2598
Fixes #2577
This replaces #2584.
Checklist