Repository: TileDB-Inc/TileDB-Py
Pull request #2041 (2024-08-20 11:25:36)

Improve Query Condition error messages to align with TileDB-SOMA-Py
Following https://github.com/single-cell-data/TileDB-SOMA/pull/2830, https://github.com/single-cell-data/TileDB-SOMA/pull/2831, and https://github.com/single-cell-data/TileDB-SOMA/pull/2864 this PR improves some `QueryCondition` error messages.
In short, it improves error messages:
- when targeted attribute contains `.`
- on non-constant query-condition RHS
- for invalid operators in query-condition parser

---

[sc-52811]
[sc-52812]
[sc-52813]


Patch:
diff --git a/tiledb/query_condition.py b/tiledb/query_condition.py
index 5acb47e9..39e722f2 100644
--- a/tiledb/query_condition.py
+++ b/tiledb/query_condition.py
@@ -118,7 +118,7 @@ class QueryCondition:
         if not isinstance(self.c_obj, qc.PyQueryCondition):
             raise TileDBError(
                 "Malformed query condition statement. A query condition must "
-                "be made up of one or more Boolean expressions."
+                "be made up of one or more boolean expressions."
             )
 
 
@@ -164,9 +164,22 @@ class QueryConditionTree(ast.NodeVisitor):
     def visit_NotIn(self, node):
         return node
 
+    def visit_Is(self, node):
+        raise TileDBError("the `is` operator is not supported")
+
+    def visit_IsNot(self, node):
+        raise TileDBError("the `is not` operator is not supported")
+
     def visit_List(self, node):
         return list(node.elts)
 
+    def visit_Attribute(self, node) -> qc.PyQueryCondition:
+        raise TileDBError(
+            f"Unhandled dot operator in {ast.dump(node)} -- if your attribute name "
+            'has a dot in it, e.g. `orig.ident`, please wrap it with `attr("...")`, '
+            'e.g. `attr("orig.ident")`'
+        )
+
     def visit_Compare(self, node: Type[ast.Compare]) -> qc.PyQueryCondition:
         operator = self.visit(node.ops[0])
 
@@ -219,6 +232,9 @@ class QueryConditionTree(ast.NodeVisitor):
             op = qc.TILEDB_IN if isinstance(operator, ast.In) else qc.TILEDB_NOT_IN
             result = self.create_pyqc(dtype)(self.ctx, node.left.id, values, op)
 
+        else:
+            raise TileDBError(f"unrecognized operator in <<{ast.dump(node)}>>")
+
         return result
 
     def aux_visit_Compare(
@@ -344,20 +360,10 @@ class QueryConditionTree(ast.NodeVisitor):
 
         if isinstance(value_node, ast.Constant):
             value = value_node.value
-        elif isinstance(value_node, ast.Constant):
-            # deprecated in 3.8
-            value = value_node.value
-        elif isinstance(value_node, ast.Constant):
-            # deprecated in 3.8
-            value = value_node.n
-        elif isinstance(value_node, ast.Constant) or isinstance(
-            value_node, ast.Constant
-        ):
-            # deprecated in 3.8
-            value = value_node.s
         else:
             raise TileDBError(
-                f"Incorrect type for comparison value: {ast.dump(value_node)}"
+                f"Incorrect type for comparison value: {ast.dump(value_node)}: right-hand sides must be constant"
+                " expressions, not variables -- did you mean to quote the right-hand side as a string?"
             )
 
         return value
@@ -425,8 +431,12 @@ class QueryConditionTree(ast.NodeVisitor):
         result = self.visit(node.left)
         rhs = node.right[1:] if isinstance(node.right, list) else [node.right]
         for value in rhs:
-            result = result.combine(self.visit(value), op)
-
+            visited = self.visit(value)
+            if not isinstance(result, qc.PyQueryCondition):
+                raise Exception(
+                    f"Unable to parse expression component {ast.dump(node)} -- did you mean to quote it as a string?"
+                )
+            result = result.combine(visited, op)
         return result
 
     def visit_BoolOp(self, node: ast.BoolOp) -> qc.PyQueryCondition:


Test patch:
diff --git a/tiledb/tests/test_query_condition.py b/tiledb/tests/test_query_condition.py
index b47f69be..07549482 100644
--- a/tiledb/tests/test_query_condition.py
+++ b/tiledb/tests/test_query_condition.py
@@ -208,6 +208,13 @@ class QueryConditionTest(DiskTestCase):
 
     def test_string_sparse(self):
         with tiledb.open(self.create_input_array_UIDSA(sparse=True)) as A:
+            with self.assertRaises(tiledb.TileDBError) as exc_info:
+                A.query(cond="S == c", attrs=["S"])[:]
+            assert (
+                "right-hand sides must be constant expressions, not variables -- did you mean to quote the right-hand side as a string?"
+                in str(exc_info.value)
+            )
+
             result = A.query(cond="S == 'c'", attrs=["S"])[:]
             assert len(result["S"]) == 1
             assert result["S"][0] == b"c"
@@ -224,6 +231,13 @@ class QueryConditionTest(DiskTestCase):
 
     def test_string_dense(self):
         with tiledb.open(self.create_input_array_UIDSA(sparse=False)) as A:
+            with self.assertRaises(tiledb.TileDBError) as exc_info:
+                A.query(cond="S == ccc", attrs=["S"])[:]
+            assert (
+                "right-hand sides must be constant expressions, not variables -- did you mean to quote the right-hand side as a string?"
+                in str(exc_info.value)
+            )
+
             result = A.query(cond="S == 'ccc'", attrs=["S"])[:]
             assert all(self.filter_dense(result["S"], A.attr("S").fill) == b"c")
 
@@ -594,6 +608,24 @@ class QueryConditionTest(DiskTestCase):
                 exc_info.value
             )
 
+    @pytest.mark.parametrize(
+        "expression_and_message",
+        [
+            ["foo is True", "the `is` operator is not supported"],
+            ["foo is not True", "the `is not` operator is not supported"],
+            [
+                "foo &&& bar",
+                "Could not parse the given QueryCondition statement: foo &&& bar",
+            ],
+        ],
+    )
+    @pytest.mark.parametrize("sparse", [True, False])
+    def test_not_supported_operators(self, expression_and_message, sparse):
+        with tiledb.open(self.create_input_array_UIDSA(sparse=sparse)) as A:
+            expression, message = expression_and_message
+            with self.assertRaisesRegex(tiledb.TileDBError, message):
+                A.query(cond=expression)[:]
+
     @pytest.mark.skipif(not has_pandas(), reason="pandas>=1.0,<3.0 not installed")
     def test_dense_datetime(self):
         import pandas as pd
@@ -753,6 +785,37 @@ class QueryConditionTest(DiskTestCase):
             # ensures that ' a' does not match 'a'
             assert len(result["A"]) == 0
 
+    def test_attribute_with_dot(self):
+        path = self.path("test_with_dot")
+        dom = tiledb.Domain(tiledb.Dim(name="dim", domain=(0, 10), dtype=np.uint32))
+        attrs = [
+            tiledb.Attr(name="attr.one", dtype=np.uint32),
+            tiledb.Attr(name="attr.two", dtype=np.uint32),
+        ]
+        schema = tiledb.ArraySchema(domain=dom, attrs=attrs, sparse=True)
+        tiledb.Array.create(path, schema)
+        with tiledb.open(path, "w") as A:
+            A[np.arange(11)] = {"attr.one": np.arange(11), "attr.two": np.arange(11)}
+        with tiledb.open(path, "r") as A:
+            with pytest.raises(tiledb.TileDBError) as exc_info:
+                A.query(cond="attr.one < 6")[:]
+            assert (
+                "TileDBError: Unhandled dot operator in Attribute(value=Name(id='attr', ctx=Load()), attr='one', ctx=Load()) -- if your attribute name has a dot in it, e.g. `orig.ident`, please wrap it with `attr(\"...\")`, e.g. `attr(\"orig.ident\")`"
+                in str(exc_info.value)
+            )
+            with pytest.raises(tiledb.TileDBError) as exc_info:
+                A.query(cond="attr.two >= 6")[:]
+            assert (
+                "TileDBError: Unhandled dot operator in Attribute(value=Name(id='attr', ctx=Load()), attr='two', ctx=Load()) -- if your attribute name has a dot in it, e.g. `orig.ident`, please wrap it with `attr(\"...\")`, e.g. `attr(\"orig.ident\")`"
+                in str(exc_info.value)
+            )
+
+            # now test with the correct syntax
+            result = A.query(cond='attr("attr.one") < 6')[:]
+            assert_array_equal(result["attr.one"], A[:6]["attr.one"])
+            result = A.query(cond='attr("attr.two") >= 6')[:]
+            assert_array_equal(result["attr.two"], A[6:]["attr.two"])
+
     @pytest.mark.skipif(not has_pandas(), reason="pandas>=1.0,<3.0 not installed")
     def test_do_not_return_attrs(self):
         with tiledb.open(self.create_input_array_UIDSA(sparse=True)) as A: