-
Notifications
You must be signed in to change notification settings - Fork 444
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
Implement multimodal( FTS, vector, hybrid) search capability #463
base: main
Are you sure you want to change the base?
Implement multimodal( FTS, vector, hybrid) search capability #463
Conversation
- Add new multiModalSearch function in lanceTableWrapper - Support vector, text, and hybrid search modes - Integrate with MainSidebar component for user interface - Update DBResultPreview and DBSearchPreview to handle new result format - Improve search result display with conditional rendering fixes 86
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.
PR Summary
This PR implements multimodal search capability, adding support for vector, text, and hybrid search modes across several components. The changes address the issue of providing a literal text search option alongside the existing AI-based similarity search.
- Added
multiModalSearch
function inLanceDBTableWrapper
class, supporting vector, text, and hybrid search types - Introduced new IPC handler for multimodal search in
ipcHandlers.ts
- Updated
SearchComponent
with a dropdown to select search type (vector, text, or hybrid) - Modified
DBSearchPreview
component to conditionally render similarity scores for vector search results - Improved search results display in
SearchComponent
, separating vector and text results
7 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -37,6 +37,24 @@ export const registerDBSessionHandlers = (store: Store<StoreSchema>, _windowMana | |||
return searchResults | |||
}) | |||
|
|||
ipcMain.handle( |
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.
style: The multi-modal search handler is implemented correctly, but consider adding error handling for invalid searchType values.
? `${filter} AND content LIKE '%${sanitizedTextQuery}%'` | ||
: `content LIKE '%${sanitizedTextQuery}%'` |
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.
style: LIKE with wildcards on both sides can be slow for large datasets. Consider using full-text search capabilities if available in LanceDB
vectorResults = rawVectorResults | ||
.map(convertRecordToDBType<DBQueryResult>) | ||
.filter((r): r is DBQueryResult => r !== null) |
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.
logic: Filtering out null results here may lead to fewer results than the specified limit. Consider adjusting the limit to account for potential null results
limit: number, | ||
searchType: 'vector' | 'text' | 'hybrid', | ||
filter?: string, | ||
) => Promise<{ vectorResults: DBQueryResult[]; textResults: DBQueryResult[] }> |
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.
style: Consider adding a type alias for the return type of multiModalSearch for better readability and maintainability.
{entry._distance != null && ( | ||
<> | ||
{/* eslint-disable-next-line no-underscore-dangle */}| Similarity:{' '} | ||
{cosineDistanceToPercentage(entry._distance)}%{' '} | ||
</> | ||
)}{' '} |
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.
style: Consider moving this logic to a separate function for better readability and reusability
{cosineDistanceToPercentage(entry._distance)}%{' '} | ||
</> | ||
)}{' '} | ||
| {modified && <span className="text-xs text-gray-400">Modified {modified}</span>} |
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.
style: Add a space before the pipe character for consistent formatting
const [searchResults, setSearchResults] = useState<{ vectorResults: DBQueryResult[]; textResults: DBQueryResult[] }>({ | ||
vectorResults: [], | ||
textResults: [], | ||
}) |
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.
style: Consider using a more specific type for searchResults instead of an inline object type
{searchResults?.textResults?.length > 0 && ( | ||
<div className="mt-4 w-full"> | ||
<h3 className="mb-2 text-white">Text Search Results</h3> | ||
{searchResults.textResults.map((result, index) => ( | ||
// eslint-disable-next-line react/no-array-index-key | ||
<DBSearchPreview key={`text-${index}`} dbResult={result} onSelect={openFileSelectSearch} /> | ||
))} | ||
</div> | ||
)} |
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.
style: Add error handling for cases where searchResults is undefined
thanking you for attempting this @subin-chella imo this should be solved with a full text search index not sql expression like you have implemented in pr. @milaiwi has been working on this exact implementation so would be good to hear thoughts |
fixes Is there a search for text option? #86
Test:
https://github.com/user-attachments/assets/ca162f51-e005-4840-a3be-3ce72a7438f2