Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion redisvl/query/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -1569,5 +1569,5 @@ def _build_query_string(self) -> str:
text = "(" + " | ".join(field_queries) + ")"

if filter_expression and filter_expression != "*":
text += f" AND {filter_expression}"
text += f" {filter_expression}"

Copy link
Copy Markdown
Collaborator

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 AND term but leaves a second precedence bug live. Redis binds | more loosely than whitespace intersection, so an unparenthesised multi-clause filter escapes the intersection. Unlike the AND bug, this one is not masked by the default stopword list, so it affects every user rather than only STOPWORDS 0 indexes:

FT.CREATE demo ON HASH PREFIX 1 e: SCHEMA text TEXT a TAG b TAG
HSET e:1 text "the quick brown fox" a x b n
HSET e:2 text "nothing here"        a n b y

FT.SEARCH demo '@text:(fox) @a:{x} | @b:{y}'   DIALECT 2   ->  e:1, e:2
FT.SEARCH demo '@text:(fox) (@a:{x} | @b:{y})' DIALECT 2   ->  e:1

e:2 contains neither fox nor a:{x}. FT.EXPLAIN on the first form shows why:

UNION {
  INTERSECT { @text:fox, TAG:@a { x } }
  TAG:@b { y }
}

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 AND was 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_expression already parenthesises composites, so (Tag("a") == "x") | (Tag("b") == "y") was never at risk.

return text
2 changes: 1 addition & 1 deletion redisvl/utils/full_text_query_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same parenthesisation here — query += f" ({filter_expression})".

The impact is lower than in TextQuery, because the text clause is ~-optional so INTERSECT{~T, X} returns the same document set as X whichever way the union binds. But the query plan is still wrong, and this helper is shared by both AggregateHybridQuery and the native HybridQuery, so I'd rather the two sites stay consistent than have one carry a latent precedence bug.


return query + ")"

Expand Down
98 changes: 97 additions & 1 deletion tests/integration/test_stopwords_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 filter_expression=(Tag("team") == "docs") | (Tag("team") == "legal") — though note that a FilterExpression union is self-parenthesising, so to exercise the actual bug the filter needs to be a raw string: filter_expression="@team:{docs} | @team:{legal}", with a third document that matches neither the text nor either tag. Without the fix that document comes back; with it, it doesn't.

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"
14 changes: 11 additions & 3 deletions tests/unit/test_aggregation_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ def test_hybrid_query_with_string_filter():
# Check that the generated query string includes both text search and filter
query_string = str(hybrid_query)
assert f"@{text_field_name}:(search | document | 12345)" in query_string
assert f"AND {string_filter}" in query_string
assert (
f"@{text_field_name}:(search | document | 12345) {string_filter}"
in query_string
)
assert " AND " not in query_string

# Test with FilterExpression - should also work (existing functionality)
filter_expression = Tag("category") == "tech"
Expand All @@ -181,7 +185,11 @@ def test_hybrid_query_with_string_filter():
f"@{text_field_name}:(search | document | 12345)"
in query_string_with_filter_expr
)
assert "AND @category:{tech}" in query_string_with_filter_expr
assert (
f"@{text_field_name}:(search | document | 12345) @category:{{tech}}"
in query_string_with_filter_expr
)
assert " AND " not in query_string_with_filter_expr

# Test with no filter - should only have text search
hybrid_query_no_filter = AggregateHybridQuery(
Expand All @@ -195,7 +203,7 @@ def test_hybrid_query_with_string_filter():
assert f"@{text_field_name}:(search | document | 12345)" in query_string_no_filter
assert "AND" not in query_string_no_filter

# Test with wildcard filter - should only have text search (no AND clause)
# Test with wildcard filter - should only have text search (no filter clause)
hybrid_query_wildcard = AggregateHybridQuery(
text=text,
text_field_name=text_field_name,
Expand Down
15 changes: 9 additions & 6 deletions tests/unit/test_hybrid_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def test_hybrid_query_with_all_parameters():
# Verify that the expected query pieces have been defined
assert get_query_pieces(hybrid_query) == [
"SEARCH",
"(~@description:(the | toon=>{$weight:2.0} | squad=>{$weight:1.5} | play | basketball | against | a | gang | of | aliens) AND @genre:{comedy})",
"(~@description:(the | toon=>{$weight:2.0} | squad=>{$weight:1.5} | play | basketball | against | a | gang | of | aliens) @genre:{comedy})",
"SCORER",
"TFIDF",
"YIELD_SCORE_AS",
Expand Down Expand Up @@ -385,7 +385,7 @@ def test_hybrid_query_with_string_filter():

assert get_query_pieces(hybrid_query) == [
"SEARCH",
"(~@description:(toon | squad | play | basketball | gang | aliens) AND @category:{tech|science|engineering})",
"(~@description:(toon | squad | play | basketball | gang | aliens) @category:{tech|science|engineering})",
"SCORER",
"BM25STD",
"VSIM",
Expand Down Expand Up @@ -418,7 +418,7 @@ def test_hybrid_query_with_tag_filter():

assert get_query_pieces(hybrid_query) == [
"SEARCH",
"(~@description:(toon | squad | play | basketball | gang | aliens) AND @genre:{comedy})",
"(~@description:(toon | squad | play | basketball | gang | aliens) @genre:{comedy})",
"SCORER",
"BM25STD",
"VSIM",
Expand Down Expand Up @@ -452,7 +452,8 @@ def test_hybrid_query_with_numeric_filter():
# Verify filter is included in serialized query
args = get_query_pieces(hybrid_query)
expected = "@age:[(30 +inf]"
assert args[1].endswith(f"AND {expected})") # Check text filter
assert args[1].endswith(f" {expected})") # Check text filter
assert " AND " not in args[1]
assert args[8] == expected # Check vector filter


Expand All @@ -472,7 +473,8 @@ def test_hybrid_query_with_text_filter():
# Verify filter is included in serialized query
args = get_query_pieces(hybrid_query)
expected = '@job:("engineer")'
assert args[1].endswith(f"AND {expected})") # Check text filter
assert args[1].endswith(f" {expected})") # Check text filter
assert " AND " not in args[1]
assert args[8] == expected # Check vector filter


Expand All @@ -492,7 +494,8 @@ def test_hybrid_query_with_combined_filters():
# Verify both filters are included in serialized query
args = get_query_pieces(hybrid_query)
expected = "(@genre:{comedy} @rating:[(7.0 +inf])"
assert args[1].endswith(f"AND {expected})") # Check text filter
assert args[1].endswith(f" {expected})") # Check text filter
assert " AND " not in args[1]
assert args[8] == expected # Check vector filter


Expand Down
14 changes: 11 additions & 3 deletions tests/unit/test_query_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,11 @@ def test_text_query_with_string_filter():
# Check that the generated query string includes both text search and filter
query_string = str(text_query)
assert f"@{text_field_name}:(search | document | 12345)" in query_string
assert f"AND {string_filter}" in query_string
assert (
f"@{text_field_name}:(search | document | 12345) {string_filter}"
in query_string
)
assert " AND " not in query_string

# Test with FilterExpression - should also work (existing functionality)
filter_expression = Tag("category") == "tech"
Expand All @@ -319,7 +323,11 @@ def test_text_query_with_string_filter():
f"@{text_field_name}:(search | document | 12345)"
in query_string_with_filter_expr
)
assert "AND @category:{tech}" in query_string_with_filter_expr
assert (
f"@{text_field_name}:(search | document | 12345) @category:{{tech}}"
in query_string_with_filter_expr
)
assert " AND " not in query_string_with_filter_expr

# Test with no filter - should only have text search
text_query_no_filter = TextQuery(
Expand All @@ -331,7 +339,7 @@ def test_text_query_with_string_filter():
assert f"@{text_field_name}:(search | document | 12345)" in query_string_no_filter
assert "AND" not in query_string_no_filter

# Test with wildcard filter - should only have text search (no AND clause)
# Test with wildcard filter - should only have text search (no filter clause)
text_query_wildcard = TextQuery(
text=text,
text_field_name=text_field_name,
Expand Down
Loading