Cosmos: strip implicit casts to allow vector search over arrays#34437
Cosmos: strip implicit casts to allow vector search over arrays#34437roji merged 4 commits intodotnet:mainfrom
Conversation
ajcvickers
force-pushed
the
CosmosVectorStuff
branch
from
258020d to
5136903
Compare
|
@roji This is now hacked to allow both arrays and read-only-memories without appropriate overloads or type mappings. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Your changes look OK, at least for now! See CI test failures, plus you have to approve it (it's my PR originally)
| /// </summary> | ||
| public override JToken? GenerateJToken(object? value) | ||
| { | ||
| // This is a hack to allow both arrays and ROM types without different function overloads or type mappings. |
There was a problem hiding this comment.
Doesn't actually seem all that hacky to me! We basically need to do ToArray() because the Cosmos SDK doesn't (yet) natively support ROM, right?
There was a problem hiding this comment.
It's hacky in the sense that a type mapping with a converter should always be able to pass its value to that converter, but in this case we're using the same type mapping for values that may or may not need converting.
ajcvickers
force-pushed
the
CosmosVectorStuff
branch
from
5136903 to
c8a5471
Compare
|
@roji I tweaked the query changes to fix the failing tests. Can you have a quick look? |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks @ajcvickers, LGTM (up to you to approve).
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #34402 Co-authored-by: Arthur Vickers <ajcvickers@hotmail.com>
Fixes #34402 Co-authored-by: Arthur Vickers <ajcvickers@hotmail.com>
2 participants
This is a targeted fix to Cosmos, to strip convert nodes out which represent implicit conversions.
I think we need to revisit our general casting/conversion strategy (across both Cosmos and relational), but this specific change makes sense to me...
@ajcvickers can you verify that all vector search tests now pass?