Skip to content

Commit e907d18

Browse files
Manisha4Manisha4claude
authored
fix: Valkey vector search - remove unsupported SORTBY (#356)
* fix: Valkey vector search - remove unsupported SORTBY and fix tag filter syntax Valkey Search KNN queries return results pre-sorted by distance, so explicit SORTBY is not supported and causes a ResponseError. This removes the .sort_by() call from the query builder. Additionally, fixes the project tag filter to use unquoted syntax with backslash escaping for special characters (e.g. hyphens, dots) instead of the quoted syntax which was returning empty results. Updates unit tests to reflect both changes: replaces three metric-specific sort order tests with a single test asserting no SORTBY is set, and updates escaping assertions to match the new backslash-escape approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply ruff format to eg_valkey.py and test_valkey.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Manisha4 <Manisha4@github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f689d3e commit e907d18

2 files changed

Lines changed: 19 additions & 79 deletions

File tree

‎sdk/python/feast/infra/online_stores/eg_valkey.py‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1138,28 +1138,22 @@ def _execute_vector_search(
11381138
Returns:
11391139
List of (doc_key, distance) tuples
11401140
"""
1141-
# Escape double quotes in project name for DIALECT 2 quoted tag syntax
1142-
# This handles special characters like hyphens which would otherwise
1143-
# be interpreted as operators (e.g., "my-project" -> "my NOT project")
1144-
escaped_project = project.replace('"', '\\"')
1141+
# Escape special characters in project name for tag filter.
1142+
# In Valkey Search tag queries, characters like - . @ need backslash escaping.
1143+
escaped_project = project
1144+
for ch in r'\-.@+~<>{}[]^":|!*()':
1145+
escaped_project = escaped_project.replace(ch, f"\\{ch}")
11451146

1146-
# Build KNN query with project filter using quoted tag syntax (DIALECT 2)
11471147
query_str = (
1148-
f'(@__project__:{{"{escaped_project}"}})'
1148+
f"(@__project__:{{{escaped_project}}})"
11491149
f"=>[KNN {top_k} @{vector_field_name} $vec AS __distance__]"
11501150
)
11511151

1152-
# Determine sort order based on metric:
1153-
# - COSINE, L2: lower distance = more similar → ascending
1154-
# - IP (Inner Product): higher score = more similar → descending
1155-
sort_ascending = metric.upper() != "IP"
1156-
1152+
# KNN results are already sorted by distance (ascending) by the engine.
1153+
# No explicit SORTBY is needed — Valkey Search does not support SORTBY
1154+
# with KNN queries.
11571155
query = (
1158-
Query(query_str)
1159-
.return_fields("__distance__")
1160-
.sort_by("__distance__", asc=sort_ascending)
1161-
.paging(0, top_k)
1162-
.dialect(2)
1156+
Query(query_str).return_fields("__distance__").paging(0, top_k).dialect(2)
11631157
)
11641158

11651159
try:

‎sdk/python/tests/unit/infra/online_store/test_valkey.py‎

Lines changed: 9 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1627,7 +1627,7 @@ def store(self):
16271627
return EGValkeyOnlineStore()
16281628

16291629
def test_project_name_with_hyphen_is_escaped(self, store):
1630-
"""Test that project names with hyphens are properly escaped in queries."""
1630+
"""Test that project names with hyphens are backslash-escaped in queries."""
16311631
from unittest.mock import MagicMock
16321632

16331633
mock_client = MagicMock()
@@ -1645,17 +1645,15 @@ def test_project_name_with_hyphen_is_escaped(self, store):
16451645
metric="COSINE",
16461646
)
16471647

1648-
# Verify the query was called
16491648
mock_client.ft.return_value.search.assert_called_once()
16501649
call_args = mock_client.ft.return_value.search.call_args
16511650
query = call_args[0][0]
16521651

1653-
# The query string should have quoted project name for DIALECT 2
1654-
# This prevents hyphen from being interpreted as negation
1655-
assert '"my-project"' in query.query_string()
1652+
# Hyphen should be backslash-escaped to prevent interpretation as negation
1653+
assert r"my\-project" in query.query_string()
16561654

16571655
def test_project_name_with_double_quote_is_escaped(self, store):
1658-
"""Test that double quotes in project names are escaped."""
1656+
"""Test that double quotes in project names are backslash-escaped."""
16591657
from unittest.mock import MagicMock
16601658

16611659
mock_client = MagicMock()
@@ -1677,11 +1675,11 @@ def test_project_name_with_double_quote_is_escaped(self, store):
16771675
call_args = mock_client.ft.return_value.search.call_args
16781676
query = call_args[0][0]
16791677

1680-
# Double quote should be escaped
1678+
# Double quote should be backslash-escaped
16811679
assert r"\"" in query.query_string()
16821680

1683-
def test_sort_ascending_for_cosine_metric(self, store):
1684-
"""Test that COSINE metric uses ascending sort (lower = better)."""
1681+
def test_no_sortby_in_knn_query(self, store):
1682+
"""Test that KNN queries do not use SORTBY (engine sorts by distance automatically)."""
16851683
from unittest.mock import MagicMock
16861684

16871685
mock_client = MagicMock()
@@ -1702,60 +1700,8 @@ def test_sort_ascending_for_cosine_metric(self, store):
17021700
call_args = mock_client.ft.return_value.search.call_args
17031701
query = call_args[0][0]
17041702

1705-
# COSINE should sort ascending (lower distance = more similar)
1706-
# Query._sortby is a SortbyField object with .args = [field, "ASC"/"DESC"]
1707-
assert query._sortby.args[0] == "__distance__"
1708-
assert query._sortby.args[1] == "ASC"
1709-
1710-
def test_sort_ascending_for_l2_metric(self, store):
1711-
"""Test that L2 metric uses ascending sort (lower = better)."""
1712-
from unittest.mock import MagicMock
1713-
1714-
mock_client = MagicMock()
1715-
mock_result = MagicMock()
1716-
mock_result.docs = []
1717-
mock_client.ft.return_value.search.return_value = mock_result
1718-
1719-
store._execute_vector_search(
1720-
client=mock_client,
1721-
index_name="test_index",
1722-
project="test_project",
1723-
vector_field_name="embedding",
1724-
embedding_bytes=b"\x00" * 16,
1725-
top_k=10,
1726-
metric="L2",
1727-
)
1728-
1729-
call_args = mock_client.ft.return_value.search.call_args
1730-
query = call_args[0][0]
1731-
1732-
# L2 should sort ascending (lower distance = more similar)
1733-
assert query._sortby.args[1] == "ASC"
1734-
1735-
def test_sort_descending_for_ip_metric(self, store):
1736-
"""Test that IP (Inner Product) metric uses descending sort (higher = better)."""
1737-
from unittest.mock import MagicMock
1738-
1739-
mock_client = MagicMock()
1740-
mock_result = MagicMock()
1741-
mock_result.docs = []
1742-
mock_client.ft.return_value.search.return_value = mock_result
1743-
1744-
store._execute_vector_search(
1745-
client=mock_client,
1746-
index_name="test_index",
1747-
project="test_project",
1748-
vector_field_name="embedding",
1749-
embedding_bytes=b"\x00" * 16,
1750-
top_k=10,
1751-
metric="IP",
1752-
)
1753-
1754-
call_args = mock_client.ft.return_value.search.call_args
1755-
query = call_args[0][0]
1756-
1757-
# IP should sort descending (higher score = more similar)
1758-
assert query._sortby.args[1] == "DESC"
1703+
# KNN results are sorted by the engine; no explicit SORTBY should be set
1704+
assert query._sortby is None
17591705

17601706
def test_default_distance_is_infinity_not_zero(self, store):
17611707
"""Test that missing __distance__ defaults to infinity, not 0.0."""

0 commit comments

Comments
 (0)