Repository navigation
Update findAndRerank sort contract and add sort parser - #2673
Draft
Hazel-Datastax wants to merge 7 commits into
Draft
Hazel-Datastax wants to merge 7 commits into
Hazel-Datastax wants to merge 7 commits into
Conversation
The command keeps the sort as the user sent it, in FindAndRerankSortDefinition: $hybrid is still read by the deserializer, any other sort is kept as raw JSON. The resolver parses the sort with FindAndRerankSortParser before it calls the planner, so a syntax error in a single leg sort is reported with a SortException code, before the errors the planner finds. FindAndRerankSort is now a sum type: MultiLeg for $hybrid, and VectorLeg, LexicalLeg or FilterLeg, built from SortExpression2, for a single leg sort. RerankLegMode is removed. Single leg sorts are still rejected with UNSUPPORTED_FIND_AND_RERANK_SORT until GH #2602 and GH #2603.
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)
|
The command keeps the sort as the user sent it. FindAndRerankSortDefinition only wraps the JSON, so reading a request never fails because of the sort. The custom deserializer is removed. FindAndRerankSortParser parses the whole sort, $hybrid included. It also checks that a $hybrid sort has a value for the vector leg, which the planner did before. The metrics features come from the field names of the sort. Behaviour changes: - A $hybrid sort with a syntax error gets a SortException code, not REQUEST_STRUCTURE_MISMATCH. MCP now gets the full error too. - A sort that is not an object gets SORT_CLAUSE_INVALID. - A $hybrid sort with no value fails before the option errors.
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
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:
This PR adds the single leg sorts to the findAndRerank sort contract:
$vectorize,$vector,$lexicaland field sorts such as{"age": -1}. The command now reads them and checks their syntax.Out of scope:
UNSUPPORTED_FIND_AND_RERANK_SORT.rerankCandidateLimitoption.Problems I ran into when adding single leg sorts:
1. The old sort class cannot hold single leg sorts.
On main,
FindAndRerankSortis one class with a leg mode and a nullable field for each value. #2647 added the single leg values to this class, and it showed three problems:{"$lexical": null}and{}are stored the same way. So a wrong$lexicalwould run as a filter leg with no sort, and the user gets no error.{"age": -1}has no field to go in.So the sort is now a sum type: a sealed interface with one record for each valid form.
2. The sort was parsed in a Jackson deserializer.
REQUEST_STRUCTURE_MISMATCH. On MCP the user gets "unable to convert value" or "Internal error", with no error code and no reason.findsort syntax, but thefindrules cannot be reused. Infind, the syntax checks are not separate code.CollectionSortClauseBuilderchecks the syntax, checks the lexical index of the collection and builds the find sort model, all in one method. So it needs the collection, and the deserializer does not have it.$vectorizesort can also be changed in place.$hybridsort were in two classes, so you had to read both to know which sorts are valid. For example, both of these sorts have a bad$vectorizevalue:{"$hybrid": {"$vectorize": 1}}: the deserializer rejected it withREQUEST_STRUCTURE_MISMATCH, because the value is not text.{"$hybrid": {"$vectorize": ""}}: the deserializer accepted it. Later, after the option checks, the planner rejected it withMISSING_HYBRID_SORT_VALUE."$vectorize": nulland a missing$vectorizegive the same result. So the metrics features had to be recorded inside the deserializer.Proposed design
If we treat the sort as a small query language, we can reuse a design from databases. A database handles a query in separate steps. I propose the same steps for the findAndRerank sort.
NOTE: This PR uses these steps only for the sort. But we can apply the same idea to the whole findAndRerank command (e.g.
hybridLimits)flowchart LR J["Jackson<br/>(HTTP and MCP)<br/>keeps the sort as sent"] --> P subgraph R["FindAndRerankCommandResolver: calls each step in order"] P["Parser (this PR)<br/>is the syntax valid?"] --> B["Binder (next PR)<br/>can it run with this<br/>collection and config?"] B --> PL["Planner<br/>how to run it?"] PL --> BU["Builder<br/>makes the operation"] end BU --> E["Execution<br/>reads, then reranks"] classDef next stroke-dasharray: 5 5 class B nextHow this fixes the problems above:
-
FindAndRerankSortDefinitionuses@JsonCreator(mode = DELEGATING), the same asSortDefinitionin find. Jackson only wraps the JSON, so there is no custom deserializer.FindAndRerankSortis a sealed interface with one record for each valid form. A wrong$lexicalis a syntax error, not an empty sort.Next PRs
If we agree on the design, I plan these PRs:
rerankCandidateLimitoption, which only single leg sorts use.hybridLimits. The parser checks the shape, for example that the object has both$vectorand$lexical. The binder checks the value against the range in the config, by default 1 to 100. Its error codes change, and MCP gets them too.Which issue(s) this PR fixes:
Fixes #
Checklist