-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix bfs search #826
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
Open
galshubeli
wants to merge
10
commits into
getzep:main
Choose a base branch
from
FalkorDB:fix-bfs-search
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix bfs search #826
Conversation
This file contains hidden or 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
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.
Important
Looks good to me! 👍
Reviewed everything up to dc03883 in 1 minute and 26 seconds. Click for details.
- Reviewed
38
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. graphiti_core/models/nodes/node_db_queries.py:130
- Draft comment:
Extra blank lines were added inside the triple-quoted query string. Remove them if not intentional to keep the query format clean. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold80%
None
2. graphiti_core/search/search_utils.py:268
- Draft comment:
Replaced inline label union in the MATCH clause with a WHERE clause check. Ensure that this change does not impact index usage on the uuid property—verify that nodes with either label (Entity or Episodic) still use the appropriate indexes. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold80%
None
3. graphiti_core/search/search_utils.py:412
- Draft comment:
Similarly, the node BFS search now uses a WHERE clause for label filtering and additional group_id checks. Confirm that the conditions 'AND n.group_id = origin.group_id' and 'AND origin.group_id IN $group_ids' are correct and do not inadvertently filter out valid nodes. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold80%
None
4. graphiti_core/models/nodes/node_db_queries.py:133
- Draft comment:
Typographical issue: The closing triple quotes on line 133 contain a trailing space. Consider removing the extra space. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 80% While technically correct, trailing whitespace is a very minor issue that would typically be handled by code formatters or IDE settings. It doesn't affect functionality. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category. The trailing whitespace could potentially cause issues with string comparison or be a sign of inconsistent code formatting practices that should be addressed. While string comparison could theoretically be affected, this is in a constant that's used for database queries where trailing whitespace would be trimmed. The formatting issue is better handled by automated tools than PR comments. This comment should be deleted as it addresses an extremely minor formatting issue that doesn't impact functionality and would be better handled by automated formatting tools.
5. graphiti_core/models/nodes/node_db_queries.py:134
- Draft comment:
Observation: Extra blank lines following the closing docstring might be unintentional. Please verify if these additional newlines are required. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold80%
The comment is asking the PR author to verify if the extra blank lines are intentional, which violates the rule against asking the author to confirm their intention. The comment does not provide a specific suggestion or improvement, nor does it address any of the additional rules such as ensuring code is idiomatic or DRY. Therefore, this comment should not be approved.
Workflow ID: wflow_bofmrOMPwjhO8TXO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
fixes #815 |
Any updates here? @galshubeli |
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.
Summary
Brief description of the changes in this PR.
Type of Change
Objective
For new features and performance improvements: Clearly describe the objective and rationale for this change.
Testing
Breaking Changes
If this is a breaking change, describe:
Checklist
make lint
passes)Related Issues
Closes #[issue number]
Important
Fixes BFS search logic in
search_utils.py
to correctly handleEntity
andEpisodic
nodes.edge_bfs_search
andnode_bfs_search
insearch_utils.py
.MATCH
clause to handleEntity
andEpisodic
nodes correctly.node_db_queries.py
.This description was created by
for dc03883. You can customize this summary. It will automatically update as commits are pushed.