-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cosmos: strip implicit casts to allow vector search over arrays #34437
Conversation
258020d
to
5136903
Compare
@roji This is now hacked to allow both arrays and read-only-memories without appropriate overloads or type mappings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5136903
to
c8a5471
Compare
@roji I tweaked the query changes to fix the failing tests. Can you have a quick look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ajcvickers, LGTM (up to you to approve).
src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
/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>
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?