Skip to content
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

Improve RedisVL filter handling, docs, and examples #72

Merged
merged 15 commits into from
Nov 1, 2023

Conversation

tylerhutcherson
Copy link
Collaborator

@tylerhutcherson tylerhutcherson commented Oct 30, 2023

Addresses #71 which highlights the need to support more edge case queries like "empty queries" or more sophisticated text LIKE query patterns.

  • Improves module readability and testability for the individual filter classes.
  • Adds support for various filter edge cases (described in the issue above).
  • Also fixes a bug in the index creation routine that originally ignored the overwrite=False case.
  • Adds more docstrings, unit tests and integration tests to improve our coverage on these cases.
  • Improves documentation coverage for hybrid queries and getting started.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (4e3de2d) 87.43% compared to head (ead0ecd) 87.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   87.43%   87.51%   +0.08%     
==========================================
  Files          17       17              
  Lines         899      937      +38     
==========================================
+ Hits          786      820      +34     
- Misses        113      117       +4     
Files Coverage Δ
redisvl/query/query.py 96.22% <100.00%> (+0.10%) ⬆️
redisvl/schema.py 95.78% <100.00%> (ø)
redisvl/query/filter.py 92.47% <94.36%> (+1.67%) ⬆️
redisvl/index.py 82.53% <63.15%> (-1.65%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tylerhutcherson tylerhutcherson added the enhancement New feature or request label Oct 31, 2023
@tylerhutcherson tylerhutcherson changed the title improve conditional filter handling for special cases Improve RedisVL filter handling Oct 31, 2023
@tylerhutcherson tylerhutcherson changed the title Improve RedisVL filter handling Improve RedisVL filter handling, docs, and examples Oct 31, 2023
Spartee
Spartee previously approved these changes Oct 31, 2023
Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving, however, I think we should leave out the index updating (or maybe move somewhere other than getting started). I think it makes things seem more complicated and I think we want that to be the lowest bar to cross possible.

Other than that, I pushed up a few changes. Biggest thing is the change of _set_value. Essentially, I don't think it's necessary to treat anything except for tag differently. I've made a few tests for this as well and updated the hybrid query notebook.

# TODO - enable async version of this
# check_redis_modules_exist(self._redis_conn)

if not self._fields:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be allowed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the current behavior of the synchro version of the create method, and was just missing here

@@ -499,6 +508,7 @@ def connect(self, url: Optional[str] = None, **kwargs):
ValueError: If no Redis URL is provided and REDIS_URL env var is not set.
"""
self._redis_conn = get_async_redis_connection(url, **kwargs)
return self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@@ -317,6 +318,7 @@ def connect(self, url: Optional[str] = None, **kwargs):
ValueError: If the REDIS_URL env var is not set and url is not provided.
"""
self._redis_conn = get_redis_connection(url, **kwargs)
return self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels odd given that then... it's a classmethod ... kindaof

Is this to make things more fluent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it reduces the potential lines of code for a user by allowing index = SearchIndex.from_dict(...).connect(...)

Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Spartee Spartee merged commit 474c4c8 into main Nov 1, 2023
6 checks passed
@tylerhutcherson tylerhutcherson deleted the conditional-filters branch November 3, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants