From 247e2f708c2b43b689cfb13bdff0f0451213ac86 Mon Sep 17 00:00:00 2001 From: Gil Forsyth Date: Thu, 15 Feb 2024 11:10:21 -0500 Subject: [PATCH] fix(comparison): wrap isnull equality check in parens (#8366) We wrap arguments to a sge.Equals in parens if the inputs are themselves binary operations. There are other operations we will have to wrap -- for now I'm just adding isNull to the list, but I imagine this will grow, possibly on a backend-by-backend basis. I'm going to investigate the mssql and oracle failures but they are unrelated to this PR (they fail the added test with or without the parentheses) --- ibis/backends/base/sqlglot/compiler.py | 4 +++- .../snapshots/test_sql/test_is_parens/isnull/out.sql | 6 +++++- ibis/backends/tests/test_generic.py | 12 ++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/ibis/backends/base/sqlglot/compiler.py b/ibis/backends/base/sqlglot/compiler.py index 3338d13533dc..a6da4f6b71c5 100644 --- a/ibis/backends/base/sqlglot/compiler.py +++ b/ibis/backends/base/sqlglot/compiler.py @@ -321,6 +321,8 @@ class SQLGlotCompiler(abc.ABC): ops.IntervalSubtract, ) + NEEDS_PARENS = BINARY_INFIX_OPS + (ops.IsNull,) + def __init__(self) -> None: self.agg = AggGen(aggfunc=self._aggregate) self.f = FuncGen() @@ -1192,7 +1194,7 @@ def visit_Aggregate(self, op, *, parent, groups, metrics): @classmethod def _add_parens(cls, op, sg_expr): - if isinstance(op, cls.BINARY_INFIX_OPS): + if isinstance(op, cls.NEEDS_PARENS): return paren(sg_expr) return sg_expr diff --git a/ibis/backends/impala/tests/snapshots/test_sql/test_is_parens/isnull/out.sql b/ibis/backends/impala/tests/snapshots/test_sql/test_is_parens/isnull/out.sql index 034d6a342694..1a669069f3a1 100644 --- a/ibis/backends/impala/tests/snapshots/test_sql/test_is_parens/isnull/out.sql +++ b/ibis/backends/impala/tests/snapshots/test_sql/test_is_parens/isnull/out.sql @@ -3,4 +3,8 @@ SELECT `t0`.`b` FROM `table` AS `t0` WHERE - `t0`.`a` IS NULL = `t0`.`b` IS NULL \ No newline at end of file + ( + `t0`.`a` IS NULL + ) = ( + `t0`.`b` IS NULL + ) \ No newline at end of file diff --git a/ibis/backends/tests/test_generic.py b/ibis/backends/tests/test_generic.py index 2c6d6d5838c8..d0430e859d4a 100644 --- a/ibis/backends/tests/test_generic.py +++ b/ibis/backends/tests/test_generic.py @@ -1846,3 +1846,15 @@ def test_select_mutate_with_dict(backend): expr = t.select({"a": ibis.literal(1.0)}).limit(1) backend.assert_frame_equal(result, expected) + + +@pytest.mark.broken(["mssql", "oracle"], reason="incorrect syntax") +def test_isnull_equality(con, backend, monkeypatch): + monkeypatch.setattr(ibis.options, "default_backend", con) + t = ibis.memtable({"x": ["a", "b", None], "y": ["c", None, None], "z": [1, 2, 3]}) + expr = t.mutate(out=t.x.isnull() == t.y.isnull()).order_by("z").select("out") + result = expr.to_pandas() + + expected = pd.DataFrame({"out": [True, False, True]}) + + backend.assert_frame_equal(result, expected)