fix(python)!: raise on bool(Expr) instead of silently dropping predicates (#4682)

Python's `and` / `or` / `not` and chained comparisons call `bool()` on
an `Expr`. `Expr` had no
`__bool__`, so that was always `True` and the filter silently lost a
predicate: `where(lit(2) <
col("id") < lit(5))` returned ids 0..4 (lower bound dropped),
`(col("id") > 7) and (col("g") == 0)`
returned all even ids, and `(a) or (b)` kept only `a` (see the issue for
the repro and output).

Changes (`python/python/lancedb/expr.py`,
`python/python/lancedb/query.py`):
- `Expr.__bool__` raises `TypeError("The truth value of an Expr is
ambiguous. ...")` with a hint to
use `&`, `|`, `~` with parentheses and `Expr.isin()` for membership.
`TypeError` rather than
`ValueError` because the object does not support truthiness at all
(polars `Expr` and SQLAlchemy
clauses raise `TypeError`; the codebase raises `TypeError` for
unsupported `lit()` values).
  pandas, numpy and pyarrow `Expression` use `ValueError`.
- `LanceHybridQueryBuilder` tested `if self._where:`, which would have
started raising for every
`.where(Expr)` on a hybrid query. It is now `isinstance(self._where,
Expr) or self._where`, so
an Expr is always applied and an empty SQL string is still skipped as
before.
- Notes section in the `Expr` docstring.

Not changed: no other code in `python/python/lancedb` tests the
truthiness of a filter or of an
`Expr` (checked `where`/`filter`/`predicate`/`condition` handling in
`query.py`, `table.py`,
`remote/`, `merge.py`, `functions.py`, `pydantic.py`: they use `is None`
/ `isinstance`). Cross-SDK:
`nodejs` has no `Expr` class on `main` yet, and JavaScript cannot
overload `&&` / `||` / `!`
either, so there is nothing equivalent to port; the Rust core is not
affected.

Behaviour changes to be aware of: code that already used `and` / `or` /
`not` / chained
comparisons on an `Expr` (which returned wrong rows) now raises
`TypeError`. `x in [expr, ...]`,
`any([...])` and `all([...])` on Exprs, and `==` between two `Query`
objects whose `filter` are
different Expr objects (pydantic compares the fields), now raise instead
of returning a meaningless
answer; `Expr` stays unhashable and `isinstance` / `is None` checks are
unaffected.

Tests (`python/python/tests/test_expr.py`): `TestExprTruthiness`
(`bool`, `and`, `or`, `not`,
chained comparison, `if`, list membership, `any`/`all` raise with a
helpful message; `&`, `|`, `~`
still combine) and `TestExprWhereBuilders` (plain, vector, FTS, hybrid
and async `.where(Expr)`
return the right rows, hybrid with an SQL string and with an empty
string is unchanged, chained
comparison is rejected before any query runs). With the source change
stashed 11 of the new tests
fail; with only `__bool__` added and the hybrid change reverted the
hybrid test fails with the
new `TypeError`, so it guards the second change. `test_expr.py`: 144
passed (123 before this change).

Verification caveat: `main`'s Python layer is newer than the newest
published wheel, so the tests
were run against the 0.41.0b14 native module
(`python/python/lancedb/expr.py` and `query.py` are
identical to the wheel); the full suite (`pytest python/tests
--ignore=python/tests/docs -m "not slow and not s3_test"`)
has the identical 25 failures before and after (wheel/main skew in
remote options, remote db,
index and table tests; none in expr/query tests): 2604 passed before,
2625 passed after (the 21
new tests). `ruff format --check python` and `ruff check python` are
clean. Not run: Rust build, Node tests, remote (cloud)
tests.

Fixes #4681

Co-authored-by: Xuanwo <github@xuanwo.io>

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]

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:`.

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))