diff --git a/python/python/lancedb/expr.py b/python/python/lancedb/expr.py index 80d01e29a..f65fbd21a 100644 --- a/python/python/lancedb/expr.py +++ b/python/python/lancedb/expr.py @@ -79,6 +79,13 @@ class Expr: >>> from lancedb.expr import col, lit >>> filt = (col("age") > lit(18)) & (col("name").lower() == lit("alice")) >>> proj = {"double": col("x") * lit(2)} + + Notes + ----- + An ``Expr`` has no Python truth value: ``bool(expr)`` raises ``TypeError``. + Combine expressions with ``&``, ``|`` and ``~`` (wrapping each comparison in + parentheses) instead of ``and``, ``or`` and ``not``, and write a range as + ``(col("x") > 1) & (col("x") < 5)`` rather than ``1 < col("x") < 5``. """ # Make Expr unhashable so that == returns an Expr rather than being used @@ -89,6 +96,19 @@ class Expr: self._inner = inner self._column_path = column_path + def __bool__(self) -> bool: + # An Expr describes a predicate that the query engine evaluates per row, + # so it has no Python truth value. Without this, ``and`` / ``or`` / + # ``not`` and chained comparisons (``1 < col("x") < 5``) would silently + # keep a single operand and drop the rest of the filter. + raise TypeError( + "The truth value of an Expr is ambiguous. Combine expressions with " + "'&' (and), '|' (or) and '~' (not), and wrap each comparison in " + "parentheses, e.g. (col('x') > 1) & (col('x') < 5). Do not use " + "'and', 'or', 'not', 'in' or chained comparisons such as " + "1 < col('x') < 5; use Expr.isin() for membership." + ) + # ── comparisons ────────────────────────────────────────────────────────── def __eq__(self, other: ExprLike) -> "Expr": # type: ignore[override] diff --git a/python/python/lancedb/query.py b/python/python/lancedb/query.py index 52f38d077..f888d4b42 100644 --- a/python/python/lancedb/query.py +++ b/python/python/lancedb/query.py @@ -3062,7 +3062,10 @@ class LanceHybridQueryBuilder(LanceQueryBuilder): if columns: self._vector_query.select(columns) self._fts_query.select(columns) - if self._where: + # An Expr has no truth value (it raises on bool()), so test for it + # explicitly instead of relying on truthiness; an empty SQL string is + # still skipped. + if isinstance(self._where, Expr) or self._where: self._vector_query.where(self._where, not self._postfilter) self._fts_query.where(self._where, not self._postfilter) # WAL-PK-FUSION: without the fallback, `if self._with_row_id:`. diff --git a/python/python/tests/test_expr.py b/python/python/tests/test_expr.py index 6cfc09922..5d81b10c3 100644 --- a/python/python/tests/test_expr.py +++ b/python/python/tests/test_expr.py @@ -11,6 +11,7 @@ import pytest import lancedb from lancedb.expr import Expr, col, func, lit +from lancedb.index import FTS # ── unit tests for Expr construction ───────────────────────────────────────── @@ -383,6 +384,68 @@ class TestExprRepr: {e: 1} +class TestExprTruthiness: + """``bool(Expr)`` must raise instead of silently keeping one operand.""" + + def test_bool_raises(self): + with pytest.raises(TypeError, match="truth value of an Expr is ambiguous"): + bool(col("x") == 1) + + def test_bool_error_points_to_operators(self): + with pytest.raises(TypeError, match=r"'&'.*'\|'.*'~'"): + bool(col("x") > 1) + + def test_and_raises(self): + with pytest.raises(TypeError, match="ambiguous"): + (col("id") > 7) and (col("g") == 0) # noqa: B015 + + def test_or_raises(self): + with pytest.raises(TypeError, match="ambiguous"): + (col("id") > 7) or (col("g") == 0) # noqa: B015 + + def test_not_raises(self): + with pytest.raises(TypeError, match="ambiguous"): + not (col("id") > 7) # noqa: B015 + + def test_chained_comparison_raises(self): + with pytest.raises(TypeError, match="ambiguous"): + lit(2) < col("id") < lit(5) # noqa: B015 + with pytest.raises(TypeError, match="ambiguous"): + 2 < col("id") < 5 # noqa: B015 + + def test_if_raises(self): + with pytest.raises(TypeError, match="ambiguous"): + if col("x"): + pass + + def test_membership_in_list_raises(self): + # list membership evaluates ``item == expr`` and calls bool() on the + # result, which used to be silently (and wrongly) True. + with pytest.raises(TypeError, match="ambiguous"): + col("x") in [1, 2, 3] # noqa: B015 + + def test_any_all_raise(self): + with pytest.raises(TypeError, match="ambiguous"): + any([col("x") > 1]) + with pytest.raises(TypeError, match="ambiguous"): + all([col("x") > 1]) + + def test_operators_still_combine(self): + a, b = col("id") > 7, col("g") == 0 + assert (a & b).to_sql() == "((id > 7) AND (g = 0))" + assert (a | b).to_sql() == "((id > 7) OR (g = 0))" + assert (~a).to_sql() == "NOT (id > 7)" + assert ((lit(2) < col("id")) & (col("id") < lit(5))).to_sql() == ( + "((2 < id) AND (id < 5))" + ) + + def test_identity_and_isinstance_checks_do_not_use_truthiness(self): + e = col("x") == 1 + assert e is not None + assert isinstance(e, Expr) + assert e is e + + class TestExprReflexive: def test_reflexive_eq(self): e = 1 == col("x") @@ -791,3 +854,121 @@ class TestExprDatetimeTimezoneIntegration: sql = lit(ts).to_sql() # Must encode 10:00 wall clock, not 10:00+local_offset. assert "2024-07-01 10:00:00" in sql + + +# ── where(Expr) must never rely on the truthiness of the filter ────────────── + + +@pytest.fixture +def search_table(tmp_path): + db = lancedb.connect(str(tmp_path)) + data = pa.table( + { + "id": list(range(12)), + "g": [i % 2 for i in range(12)], + "text": ["keep" if i < 9 else "other" for i in range(12)], + "vector": pa.FixedSizeListArray.from_arrays( + pa.array([float(i) for i in range(12) for _ in range(2)], pa.float32()), + 2, + ), + } + ) + table = db.create_table("search", data) + table.create_index("text", config=FTS()) + return table + + +def _ids(rows): + return sorted(r["id"] for r in rows) + + +class TestExprWhereBuilders: + def test_chained_comparison_is_rejected_before_querying(self, search_table): + # The silent bug: this used to return ids 0..4 (lower bound dropped). + with pytest.raises(TypeError, match="ambiguous"): + search_table.search().where(lit(2) < col("id") < lit(5)) + + def test_plain_where_expr(self, search_table): + where = (lit(2) < col("id")) & (col("id") < lit(5)) + rows = search_table.search().where(where).limit(100).to_list() + assert _ids(rows) == [3, 4] + + def test_vector_where_expr(self, search_table): + where = (col("id") > lit(7)) & (col("g") == lit(0)) + rows = search_table.search([0.0, 0.0]).where(where).limit(100).to_list() + assert _ids(rows) == [8, 10] + + def test_fts_where_expr(self, search_table): + where = (col("id") > lit(3)) | (col("g") == lit(1)) + rows = ( + search_table.search("keep", query_type="fts") + .where(where) + .limit(100) + .to_list() + ) + # "keep" matches ids 0..8; filter keeps id > 3 or odd ids. + assert _ids(rows) == [1, 3, 4, 5, 6, 7, 8] + + def test_hybrid_where_expr(self, search_table): + # The hybrid builder used to test ``if self._where:``; that must not + # call bool() on an Expr now that it raises. + where = (col("id") > lit(3)) & (col("g") == lit(0)) + rows = ( + search_table.search(query_type="hybrid") + .vector([0.0, 0.0]) + .text("keep") + .where(where) + .limit(100) + .to_list() + ) + # Hybrid search fuses the vector and FTS results, and the filter applies + # to both sides: the vector side alone returns every row passing it. + assert _ids(rows) == [4, 6, 8, 10] + + def test_hybrid_where_sql_string_still_works(self, search_table): + rows = ( + search_table.search(query_type="hybrid") + .vector([0.0, 0.0]) + .text("keep") + .where("id > 3 AND g = 0") + .limit(100) + .to_list() + ) + assert _ids(rows) == [4, 6, 8, 10] + + def test_hybrid_empty_where_is_ignored(self, search_table): + rows = ( + search_table.search(query_type="hybrid") + .vector([0.0, 0.0]) + .text("keep") + .where("") + .limit(100) + .to_list() + ) + assert _ids(rows) == list(range(12)) + + def test_repeated_where_expr_combines(self, search_table): + rows = ( + search_table.search() + .where(col("id") > lit(2)) + .where(col("id") < lit(5)) + .limit(100) + .to_list() + ) + assert _ids(rows) == [3, 4] + + def test_query_model_with_expr_filter(self, search_table): + query = search_table.search().where(col("id") > lit(2)).to_query_object() + assert isinstance(query.filter, Expr) + + @pytest.mark.asyncio + async def test_async_where_expr(self, tmp_path): + db = await lancedb.connect_async(str(tmp_path)) + table = await db.create_table( + "t", pa.table({"id": list(range(12)), "g": [i % 2 for i in range(12)]}) + ) + where = (col("id") > lit(7)) & (col("g") == lit(0)) + rows = await table.query().where(where).to_list() + assert _ids(rows) == [8, 10] + with pytest.raises(TypeError, match="ambiguous"): + table.query().where(lit(2) < col("id") < lit(5))