Skip to content

Commit 2a2e9af

Browse files
authored
Python: fix(sqlserver): handle None and boolean expressions in SqlServerCollection lambda parser (#14687)
### Summary Closes #14683 `SqlServerCollection._lambda_parser` rendered `None` comparisons as `[field] = ?` or `[field] <> ?` binding the string literal `'None'`. Because SQL Server evaluates equality with `NULL` under `ANSI_NULLS ON` as UNKNOWN, search filters targeting `None` could never match `NULL` rows. In addition: - `ast.Is` and `ast.IsNot` raised `NotImplementedError: Unsupported operator: <class 'ast.Is'>` on `x.tenant is None` and `x.tenant is not None`. - Booleans were bound as string `'True'` / `'False'` instead of `'1'` / `'0'` (which is expected by SQL Server `BIT` columns and is already used by `_cast_value`). - Omitted search arguments in `default_dynamic_filter_function` generating `lambda x: x.tenant == None` bound `'None'` as a query parameter. ### Changes 1. **`SqlServerCollection._lambda_parser`**: - Inspect comparison operators `(ast.Eq, ast.NotEq, ast.Is, ast.IsNot)`. - Normalize operands so if `left_node` is `ast.Constant` with `value is None`, it swaps `left_node` and `right_node`. - When comparing against `None`: - If both sides are scalar constants, evaluate to T-SQL `(1 = 1)` or `(1 = 0)`. - Otherwise, render `[column] IS NULL` (for `ast.Eq`, `ast.Is`) or `[column] IS NOT NULL` (for `ast.NotEq`, `ast.IsNot`) without appending any parameter to `command.parameters`. - Raise `NotImplementedError` if `ast.Is` or `ast.IsNot` is used with a non-`None` operand. - In `case ast.Constant():`, bind booleans as `'1'` if `True` else `'0'` to match `_cast_value` and SQL Server `BIT` semantics. - Avoid double-parsing `right_node` across comparison operator branches. 2. **Unit Tests**: - Added parameterized `test_null_filter_parser` in `tests/unit/connectors/memory/test_sql_server.py` covering 28 cases mirroring Postgres null filter parser tests (`== None`, `!= None`, `is None`, `is not None`, reversed comparisons, constant truth evaluation, compound boolean expressions, boolean bindings `'1'`/`'0'`, and string `'None'`). - Added `test_null_filter_query_parameter_order` verifying SELECT query parameter alignment when combining null filters with vector and range filters. - Added `test_dynamic_null_filter` verifying `default_dynamic_filter_function(tenant=None)` generates `[tenant] IS NULL` without parameters. - Added `test_unsupported_is_operator` verifying unsupported `is` comparisons raise `NotImplementedError`. ### Verification - `uv run pytest tests/unit/connectors/memory/test_sql_server.py` (73 passed in 0.16s). - `uv run ruff check tests/unit/connectors/memory/test_sql_server.py semantic_kernel/connectors/sql_server.py` (all checks passed). - `uv run ruff format --check tests/unit/connectors/memory/test_sql_server.py semantic_kernel/connectors/sql_server.py` (all formatted). Signed-off-by: Wasim <wasim143mr@gmail.com>
1 parent 8cda8fe commit 2a2e9af

2 files changed

Lines changed: 138 additions & 14 deletions

File tree

‎python/semantic_kernel/connectors/sql_server.py‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -571,15 +571,14 @@ async def _fetch_records(self, query: SqlCommand) -> AsyncIterable[dict[str, Any
571571
cur.execute(*query.to_execute())
572572
col_names = [desc[0] for desc in cur.description]
573573
for row in cur:
574-
record = {
574+
yield {
575575
col: (
576576
json.loads(row.__getattribute__(col))
577577
if col in self.definition.vector_field_names
578578
else row.__getattribute__(col)
579579
)
580580
for col in col_names
581581
}
582-
yield record
583582
await asyncio.sleep(0)
584583

585584
@override
@@ -599,33 +598,43 @@ def parse(node: ast.AST) -> str:
599598
op = node.ops[idx]
600599
values.append(parse(ast.Compare(left=left, ops=[op], comparators=[right])))
601600
return f"({' AND '.join(values)})"
602-
left = parse(node.left) # type: ignore
601+
left_node = node.left
603602
right_node = node.comparators[0]
604603
op = node.ops[0]
604+
if isinstance(op, (ast.Eq, ast.NotEq, ast.Is, ast.IsNot)):
605+
if isinstance(left_node, ast.Constant) and left_node.value is None:
606+
left_node, right_node = right_node, left_node
607+
if isinstance(right_node, ast.Constant) and right_node.value is None:
608+
if isinstance(left_node, ast.Constant) and (
609+
left_node.value is None or isinstance(left_node.value, (str, int, float, bool, bytes))
610+
):
611+
matches = (left_node.value is None) == isinstance(op, (ast.Eq, ast.Is))
612+
return "(1 = 1)" if matches else "(1 = 0)"
613+
return (
614+
f"{parse(left_node)} IS NULL"
615+
if isinstance(op, (ast.Eq, ast.Is))
616+
else f"{parse(left_node)} IS NOT NULL"
617+
)
618+
if isinstance(op, (ast.Is, ast.IsNot)):
619+
raise NotImplementedError(f"Unsupported operator: {type(op)}")
620+
left = parse(left_node) # type: ignore
621+
right = parse(right_node) # type: ignore
605622
match op:
606623
case ast.In():
607-
right = parse(right_node) # type: ignore
608624
return f"{left} IN {right}"
609625
case ast.NotIn():
610-
right = parse(right_node) # type: ignore
611626
return f"{left} NOT IN {right}"
612627
case ast.Eq():
613-
right = parse(right_node) # type: ignore
614628
return f"{left} = {right}"
615629
case ast.NotEq():
616-
right = parse(right_node) # type: ignore
617630
return f"{left} <> {right}"
618631
case ast.Gt():
619-
right = parse(right_node) # type: ignore
620632
return f"{left} > {right}"
621633
case ast.GtE():
622-
right = parse(right_node) # type: ignore
623634
return f"{left} >= {right}"
624635
case ast.Lt():
625-
right = parse(right_node) # type: ignore
626636
return f"{left} < {right}"
627637
case ast.LtE():
628-
right = parse(right_node) # type: ignore
629638
return f"{left} <= {right}"
630639
raise NotImplementedError(f"Unsupported operator: {type(op)}")
631640
case ast.BoolOp():
@@ -659,7 +668,10 @@ def parse(node: ast.AST) -> str:
659668
return QueryBuilder.escape_identifier(node.id)
660669
case ast.Constant():
661670
value = node.value
662-
if isinstance(value, (str, int, float, bool, bytes)) or value is None:
671+
if isinstance(value, bool):
672+
command.add_parameter("1" if value else "0")
673+
return "?"
674+
if isinstance(value, (str, int, float, bytes)) or value is None:
663675
command.add_parameter(str(value))
664676
return "?"
665677
raise VectorStoreOperationException(f"Unsupported constant type: {type(value)}")

‎python/tests/unit/connectors/memory/test_sql_server.py‎

Lines changed: 114 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import json
44
import sys
55
from dataclasses import dataclass
6-
from typing import NamedTuple
6+
from typing import Annotated, NamedTuple
77
from unittest.mock import AsyncMock, MagicMock, NonCallableMagicMock, patch
88

99
from pytest import fixture, mark, param, raises
@@ -21,12 +21,20 @@
2121
_build_select_query,
2222
_build_select_table_names_query,
2323
)
24-
from semantic_kernel.data.vector import DistanceFunction, IndexKind, VectorSearchOptions, VectorStoreField
24+
from semantic_kernel.data.vector import (
25+
DistanceFunction,
26+
IndexKind,
27+
VectorSearchOptions,
28+
VectorStoreField,
29+
default_dynamic_filter_function,
30+
vectorstoremodel,
31+
)
2532
from semantic_kernel.exceptions.vector_store_exceptions import (
2633
VectorStoreInitializationException,
2734
VectorStoreModelValidationError,
2835
VectorStoreOperationException,
2936
)
37+
from semantic_kernel.functions.kernel_parameter_metadata import KernelParameterMetadata
3038

3139

3240
class TestQueryBuilder:
@@ -626,3 +634,107 @@ async def test_no_connection(self, sql_server_unit_test_env, definition):
626634
await collection.get("1")
627635
with raises(VectorStoreOperationException):
628636
await collection.delete("1")
637+
638+
639+
@fixture
640+
def filter_collection(sql_server_unit_test_env):
641+
@vectorstoremodel
642+
@dataclass
643+
class FilterRecord:
644+
id: Annotated[int, VectorStoreField("key")]
645+
tenant: Annotated[str, VectorStoreField("data")]
646+
active: Annotated[bool, VectorStoreField("data")] = False
647+
embedding: Annotated[
648+
list[float],
649+
VectorStoreField("vector", dimensions=3, distance_function=DistanceFunction.COSINE_DISTANCE),
650+
] = None
651+
652+
return SqlServerCollection(
653+
collection_name="filter_records",
654+
record_type=FilterRecord,
655+
env_file_path="test.env",
656+
)
657+
658+
659+
@mark.parametrize(
660+
"expression, expected_sql, expected_params",
661+
[
662+
("x.tenant == None", "[tenant] IS NULL", []),
663+
("x.tenant != None", "[tenant] IS NOT NULL", []),
664+
("x.tenant is None", "[tenant] IS NULL", []),
665+
("x.tenant is not None", "[tenant] IS NOT NULL", []),
666+
("None == x.tenant", "[tenant] IS NULL", []),
667+
("None != x.tenant", "[tenant] IS NOT NULL", []),
668+
("None is x.tenant", "[tenant] IS NULL", []),
669+
("None is not x.tenant", "[tenant] IS NOT NULL", []),
670+
("None == None", "(1 = 1)", []),
671+
("None != None", "(1 = 0)", []),
672+
("None is None", "(1 = 1)", []),
673+
("None is not None", "(1 = 0)", []),
674+
("'value' != None", "(1 = 1)", []),
675+
("None == 'value'", "(1 = 0)", []),
676+
("'value' is not None", "(1 = 1)", []),
677+
("None is 'value'", "(1 = 0)", []),
678+
("0 != None", "(1 = 1)", []),
679+
("False == None", "(1 = 0)", []),
680+
("not x.tenant == None", "NOT ([tenant] IS NULL)", []),
681+
("not x.tenant != None", "NOT ([tenant] IS NOT NULL)", []),
682+
("x.tenant == None or x.id == 1", "([tenant] IS NULL OR [id] = ?)", ["1"]),
683+
("None == x.tenant == None", "([tenant] IS NULL AND [tenant] IS NULL)", []),
684+
("'value' == x.tenant != None", "(? = [tenant] AND [tenant] IS NOT NULL)", ["value"]),
685+
("x.active == True", "[active] = ?", ["1"]),
686+
("x.active == False", "[active] = ?", ["0"]),
687+
("x.active != True", "[active] <> ?", ["1"]),
688+
("x.active != False", "[active] <> ?", ["0"]),
689+
("x.tenant == 'None'", "[tenant] = ?", ["None"]),
690+
],
691+
)
692+
def test_null_filter_parser(filter_collection, expression, expected_sql, expected_params):
693+
command = filter_collection._build_filter(f"lambda x: {expression}")
694+
695+
assert str(command) == expected_sql
696+
assert command.parameters == expected_params
697+
698+
699+
def test_null_filter_query_parameter_order(filter_collection):
700+
filters = [
701+
"lambda x: x.id > 0",
702+
"lambda x: x.tenant == None or x.tenant == 'value'",
703+
"lambda x: x.id < 5",
704+
]
705+
parsed_filters = filter_collection._build_filter(filters)
706+
cmd = _build_search_query(
707+
"dbo",
708+
"filter_records",
709+
filter_collection.definition.key_field,
710+
filter_collection.definition.data_fields,
711+
filter_collection.definition.vector_fields,
712+
[1.0, 0.0, 0.0],
713+
VectorSearchOptions(vector_property_name="embedding", filter=filters),
714+
filter=parsed_filters,
715+
)
716+
717+
assert str(cmd) == (
718+
"SELECT [id], [tenant], [active], VECTOR_DISTANCE('cosine', [embedding], CAST(? AS VECTOR(3))) as "
719+
"_vector_distance_value\n"
720+
" FROM [dbo].[filter_records] \n"
721+
" WHERE [id] > ? \n"
722+
" AND ([tenant] IS NULL OR [tenant] = ?) \n"
723+
" AND [id] < ? \n"
724+
"ORDER BY _vector_distance_value ASC\n"
725+
"OFFSET 0 ROWS FETCH NEXT 3 ROWS ONLY;"
726+
)
727+
assert cmd.parameters == ["[1.0, 0.0, 0.0]", "0", "value", "5"]
728+
729+
730+
def test_dynamic_null_filter(filter_collection):
731+
filter = default_dynamic_filter_function(parameters=[KernelParameterMetadata(name="tenant")], tenant=None)
732+
clause = filter_collection._build_filter(filter)
733+
734+
assert str(clause) == "[tenant] IS NULL"
735+
assert clause.parameters == []
736+
737+
738+
def test_unsupported_is_operator(filter_collection):
739+
with raises(NotImplementedError, match="Unsupported operator"):
740+
filter_collection._build_filter("lambda x: x.tenant is 'test'")

0 commit comments

Comments
 (0)