-
Notifications
You must be signed in to change notification settings - Fork 99
fix: use whitespace intersection for filtered text queries #709
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ def build_query_string( | |
| query = f"(~@{text_field_name}:({self._tokenize_and_escape_query(text)})" | ||
|
|
||
| if filter_expression and filter_expression != "*": | ||
| query += f" AND {filter_expression}" | ||
| query += f" {filter_expression}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same parenthesisation here — The impact is lower than in |
||
|
|
||
| return query + ")" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,11 @@ | |
| import pytest | ||
|
|
||
| from redisvl.index import SearchIndex | ||
| from redisvl.query import FilterQuery | ||
| from redisvl.query import AggregateHybridQuery, FilterQuery, TextQuery | ||
| from redisvl.query.filter import Tag | ||
| from redisvl.redis.utils import array_to_buffer | ||
| from redisvl.schema import IndexSchema | ||
| from tests.conftest import skip_if_redis_version_below | ||
|
|
||
|
|
||
| @pytest.fixture | ||
|
|
@@ -91,6 +94,56 @@ def default_stopwords_index(client, default_stopwords_schema): | |
| index.delete(drop=True) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def filtered_queries_stopwords_disabled_index(redis_url, redis_test_name): | ||
| """Index with STOPWORDS 0 for filtered text and hybrid query regressions.""" | ||
| index_name = redis_test_name("filtered_queries_stopwords_disabled") | ||
| index = SearchIndex.from_dict( | ||
| { | ||
| "index": { | ||
| "name": index_name, | ||
| "prefix": f"{index_name}:", | ||
| "storage_type": "hash", | ||
| "stopwords": [], | ||
| }, | ||
| "fields": [ | ||
| {"name": "text", "type": "text"}, | ||
| {"name": "team", "type": "tag"}, | ||
| { | ||
| "name": "embedding", | ||
| "type": "vector", | ||
| "attrs": { | ||
| "dims": 2, | ||
| "distance_metric": "cosine", | ||
| "algorithm": "flat", | ||
| "datatype": "float32", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| redis_url=redis_url, | ||
| ) | ||
| index.create(overwrite=True, drop=True) | ||
| index.load( | ||
| [ | ||
| { | ||
| "text": "reference handbook", | ||
| "team": "docs", | ||
| "embedding": array_to_buffer([1.0, 0.0], "float32"), | ||
| }, | ||
| { | ||
| "text": "reference handbook", | ||
| "team": "support", | ||
| "embedding": array_to_buffer([1.0, 0.0], "float32"), | ||
| }, | ||
| ] | ||
| ) | ||
|
|
||
| yield index | ||
|
|
||
| index.delete(drop=True) | ||
|
|
||
|
|
||
| def test_create_index_with_stopwords_disabled(client, stopwords_disabled_index): | ||
| """Test creating an index with STOPWORDS 0.""" | ||
| # Verify index was created | ||
|
|
@@ -190,3 +243,46 @@ def test_stopwords_disabled_allows_searching_common_words( | |
| # With STOPWORDS 0, "of" should be indexed and searchable | ||
| assert len(results.docs) > 0 | ||
| assert any("of" in doc.title.lower() for doc in results.docs) | ||
|
|
||
|
|
||
| def test_filtered_text_query_with_stopwords_disabled( | ||
| filtered_queries_stopwords_disabled_index, | ||
| ): | ||
| """Filtered text queries should not add AND as a full-text search term.""" | ||
| query = TextQuery( | ||
| text="handbook", | ||
| text_field_name="text", | ||
| filter_expression=Tag("team") == "docs", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This filter is a single clause, so the test passes with or without the parenthesisation fix. Could you add one case with a union filter, so the precedence behaviour is pinned rather than incidental? Something like |
||
| return_fields=["text", "team"], | ||
| stopwords=None, | ||
| ) | ||
|
|
||
| results = filtered_queries_stopwords_disabled_index.query(query) | ||
|
|
||
| assert len(results) == 1 | ||
| assert results[0]["text"] == "reference handbook" | ||
| assert results[0]["team"] == "docs" | ||
|
|
||
|
|
||
| def test_filtered_aggregate_hybrid_query_with_stopwords_disabled( | ||
| filtered_queries_stopwords_disabled_index, | ||
| ): | ||
| """Filtered aggregate hybrid queries should work with STOPWORDS 0.""" | ||
| skip_if_redis_version_below( | ||
| filtered_queries_stopwords_disabled_index.client, "7.2.0" | ||
| ) | ||
| query = AggregateHybridQuery( | ||
| text="handbook", | ||
| text_field_name="text", | ||
| vector=[1.0, 0.0], | ||
| vector_field_name="embedding", | ||
| filter_expression=Tag("team") == "docs", | ||
| return_fields=["text", "team"], | ||
| stopwords=None, | ||
| ) | ||
|
|
||
| results = filtered_queries_stopwords_disabled_index.query(query) | ||
|
|
||
| assert len(results) == 1 | ||
| assert results[0]["text"] == "reference handbook" | ||
| assert results[0]["team"] == "docs" | ||
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.
Could you wrap this in parentheses —
text += f" ({filter_expression})", as your issue proposed?Whitespace alone fixes the
ANDterm but leaves a second precedence bug live. Redis binds|more loosely than whitespace intersection, so an unparenthesised multi-clause filter escapes the intersection. Unlike theANDbug, this one is not masked by the default stopword list, so it affects every user rather than onlySTOPWORDS 0indexes:e:2contains neitherfoxnora:{x}.FT.EXPLAINon the first form shows why:To be clear, this is pre-existing rather than something you're introducing — on a default-stopwords index the old string parsed to the same plan once
ANDwas dropped. But since the join is being rewritten anyway it's worth closing both at once, and the failure mode is nastier than the one you fixed: documents that match neither clause come back, which is harder to spot than zero results.Only raw-string filters are affected.
FilterExpression.format_expressionalready parenthesises composites, so(Tag("a") == "x") | (Tag("b") == "y")was never at risk.