F/dsearch queries: Add doc search sql queries #985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5f4aebc in 1 minute and 13 seconds
More details
- Looked at
987lines of code in15files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/search_docs_hybrid.py:24
- Draft comment:
The SQL query is missing a closing parenthesis. Add a closing parenthesis at the end of the query to fix the syntax error. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. agents-api/agents_api/queries/tools/delete_tool.py:28
- Draft comment:
TheForeignKeyViolationErrormight not accurately represent a missing developer or agent. Consider using a more appropriate exception or adding additional checks to verify the existence of the developer or agent before attempting deletion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative - it says "might not accurately represent" without strong evidence. In a properly designed database, foreign key constraints are the correct way to ensure referential integrity. If the developer or agent doesn't exist, the foreign key constraint will fail, which is exactly what we want. The current error handling is following standard REST API practices by returning 404 for not found resources.
Maybe there are edge cases where a foreign key violation could occur for reasons other than missing developer/agent? Maybe additional checks could improve the user experience?
While additional checks are possible, they would add complexity without clear benefit. The foreign key constraint is the authoritative source of truth, and the current error handling follows REST best practices.
Delete this comment. The current error handling is appropriate and follows good practices. The suggestion is speculative and would add unnecessary complexity.
3. agents-api/agents_api/queries/tools/get_tool.py:25
- Draft comment:
TheForeignKeyViolationErrormight not accurately represent a missing developer or agent. Consider using a more appropriate exception or adding additional checks to verify the existence of the developer or agent before attempting to get the tool. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/queries/tools/list_tools.py:29
- Draft comment:
TheForeignKeyViolationErrormight not accurately represent a missing developer or agent. Consider using a more appropriate exception or adding additional checks to verify the existence of the developer or agent before attempting to list the tools. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_9IqGyywJgBjz67jv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| metadata_filter: dict[str, Any] = {}, | ||
| search_language: str = "english", | ||
| confidence: float = 0.5, | ||
| ) -> tuple[str, list]: | ||
| """ |
There was a problem hiding this comment.
The docstring mentions owner_type and owner_id, but the function uses owners. Update the docstring to reflect the correct parameter owners which is a list of tuples containing owner type and ID.
CI Failure Feedback 🧐(Checks updated until commit 23235f4)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d16a693 in 31 seconds
More details
- Looked at
231lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/search_docs_by_embedding.py:21
- Draft comment:
The SQL query string is missing a closing parenthesis. This will cause a syntax error when executing the query. Please add a closing parenthesis at the end of the query. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/queries/docs/search_docs_by_text.py:21
- Draft comment:
The SQL query string is missing a closing parenthesis. This will cause a syntax error when executing the query. Please add a closing parenthesis at the end of the query. - Reason this comment was not posted:
Marked as duplicate.
3. agents-api/agents_api/queries/docs/search_docs_hybrid.py:25
- Draft comment:
The SQL query string is missing a closing parenthesis. This will cause a syntax error when executing the query. Please add a closing parenthesis at the end of the query. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_6XzwZVAhfWnVrSGa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement
Description
Changes walkthrough 📝
__init__.py
Add hybrid search capabilities to docs moduleagents-api/agents_api/queries/docs/init.py
exports
search_docs_by_embedding.py
Add vector search implementation and error handlingagents-api/agents_api/queries/docs/search_docs_by_embedding.py
search_docs_hybrid.py
Implement hybrid search functionalityagents-api/agents_api/queries/docs/search_docs_hybrid.py
*
Enhance tools module with error handling and formattingagents-api/agents_api/queries/tools/*
000018_doc_search.up.sql
Optimize hybrid search SQL implementationmemory-store/migrations/000018_doc_search.up.sql
Important
Add hybrid document search with improved error handling and SQL optimization, and enhance tools module with better validation.
search_docs_hybrid.py.search_docs_by_embedding.py,search_docs_by_text.py, andsearch_docs_hybrid.py.sqlglotincreate_tools.py,delete_tool.py, andget_tool.py.000018_doc_search.up.sqlto improve join conditions and removedeveloper_idfrom results.test_docs_queries.py.tools/*files.This description was created by
for d16a693. It will automatically update as commits are pushed.