Add knowledgebase implementation for Ai Azure#24
Add knowledgebase implementation for Ai Azure#24SasinduDilshara merged 10 commits intoballerina-platform:mainfrom
Conversation
SasinduDilshara
requested review from
MohamedSabthar and
shafreenAnfar
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
===========================================
- Coverage 60.74% 28.76% -31.98%
===========================================
Files 4 5 +1
Lines 349 737 +388
Branches 103 223 +120
===========================================
Hits 212 212
- Misses 137 525 +388 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaryamZi
left a comment
MaryamZi
left a comment
There was a problem hiding this comment.
Given the urgency, we can focus on the comments that have an impact on the public APIs for now.
| # + maxLimit - The maximum number of items to return | ||
| # + filters - Optional metadata filters to apply during retrieval | ||
| # + return - An array of matching chunks with similarity scores, or an `ai:Error` if retrieval fails | ||
| public isolated function retrieve(string query, int maxLimit = 10, ai:MetadataFilters? filters = ()) returns ai:QueryMatch[]|ai:Error { |
There was a problem hiding this comment.
Methods generally seem to be long. Unless not possible because of isolated, would be better to break up into functions. Will also get simplified with things like query expressions.
There was a problem hiding this comment.
Will do in another PR
| } | ||
|
|
||
| private isolated function buildODataFilter(ai:MetadataFilters filters) returns string? { | ||
| return self.convertFiltersToOData(filters); |
There was a problem hiding this comment.
We can use both approaches I guess
There was a problem hiding this comment.
Why this though? It's just another level of indirection in the current state.
SasinduDilshara
merged commit d7a2689
into
ballerina-platform:main
MaryamZi
left a comment
MaryamZi
left a comment
There was a problem hiding this comment.
@SasinduDilshara is there an issue tracking adding tests? Please create an issue to track addressing the suggestions also.
| } | ||
|
|
||
| private isolated function buildODataFilter(ai:MetadataFilters filters) returns string? { | ||
| return self.convertFiltersToOData(filters); |
There was a problem hiding this comment.
Why this though? It's just another level of indirection in the current state.
| if filterExpressions.length() == 0 { | ||
| return (); | ||
| } | ||
|
|
||
| if filterExpressions.length() == 1 { |
There was a problem hiding this comment.
filterExpressions.length() accessed repeatedly.
| conditions.push(string `${fieldName} ${ODATA_OPERATOR_EQ} ${formattedValue}`); | ||
| } | ||
| } | ||
| if conditions.length() > 0 { |
| index:IndexAction|ai:Error indexAction = createIndexAction( | ||
| doc, | ||
| embedding, | ||
| i, | ||
| self.keyFieldName, | ||
| self.contentFieldName, | ||
| self.vectorFieldNames, | ||
| self.allFields, | ||
| self.verbose | ||
| ); | ||
|
|
||
| if indexAction is ai:Error { | ||
| return indexAction; | ||
| } |
There was a problem hiding this comment.
Why aren't you using check instead?
| } else { | ||
| if isPossibleMetadata { | ||
| logIfVerboseEnabled( |
There was a problem hiding this comment.
Can be an else-if.
Can do an early return (continue) above too.
4 participants
Add knowledgebase implementation for Ai Azure