-
Notifications
You must be signed in to change notification settings - Fork 36
EF-166: Logging for zero results from vector search and other tweaks #236
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
Conversation
@rstam You were assigned as a reviewer automatically; feel free to ignore. |
8fbdcc5
to
0cb8e6b
Compare
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.
Looks great, just one small change regarding fixing an exception message formatting.
} | ||
|
||
private const string VectorSearchNeedsIndexString = | ||
"A vector query for '{entityType}.{propertyType}' could not be executed because the vector " + |
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.
Nice clear helpful error messages with next steps :)
if (vectorIndexesInModel == null || vectorIndexesInModel.Count == 0) | ||
{ | ||
ThrowForBadOptions( | ||
$"A vector query for '{entityType!.DisplayName()}.{members[0].Name}' could not be executed because the vector " + |
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.
ThrowForBadOptions emits $"A vector query for '{entityType!.DisplayName()}.{members[0].Name}' could not be executed because {reason}");
so everything up to and including "because" here should be dropped.
|
||
private sealed class VectorSearchReplacer : ExpressionVisitor | ||
{ | ||
private readonly MethodCallExpression _removed; |
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.
Nit: two spaces.
0f2c4ff
to
4fccf77
Compare
@damieng Ready for re-review. |
No description provided.