HyukjinKwon commented on code in PR #59339:
URL: https://github.com/apache/spark/pull/59339#discussion_r4265222696
##########
python/pyspark/sql/transpile.py:
##########
@@ -239,8 +329,129 @@ class CatalystTranspiler(AbstractTranspiler):
variety = "catalyst"
def __init__(self) -> None:
+ # Category inference depends on the per-variant assumptions set in
+ # ``_transpile_from_ast``; both are reset there per variant.
self._param_categories: dict[int, str] = {}
self._category_cache: dict[int, str] = {}
+ # Instance attrs (not class defaults): ``_raise_on_null`` does ``|=``
on
+ # ``_pending_null_guards``, which on a class-default set would mutate
the
+ # class dict and leak state across UDFs. Both reset per variant in
+ # ``_transpile_from_ast``.
+ self._non_null: frozenset = frozenset()
+ self._pending_null_guards: frozenset = frozenset()
+ #: Raising NULL checks the last lowered variant needed; empty when none
+ #: survived. ``_transpile_func`` unions this across kept variants and
warns
+ #: once per UDF. Assigned, not accumulated, so nothing leaks between
UDFs.
+ self.null_guards: frozenset = frozenset()
+
+ @contextlib.contextmanager
+ def _narrowed(self, proven_non_null: frozenset) -> Iterator[None]:
+ """Lower the enclosed nodes with ``proven_non_null`` added to what is
known.
+
+ Save/restore suffices: lowering walks straight down, so a fact's scope
is
+ exactly the subtree we recurse into.
+ """
+ previous = self._non_null
+ self._non_null = previous | proven_non_null
+ try:
+ yield
+ finally:
+ self._non_null = previous
+
+ def _is_never_null(self, params: List[str], node: ast.AST) -> bool:
+ """Whether ``node`` provably cannot evaluate to NULL here.
+
+ Answers "do we still need a NULL check?" Every arm is an explicit
proof and
+ the catch-all is ``False``: a missed proof costs a redundant check, a
wrong
+ one drops a check Python needs. Expressions only.
+ """
+ match node:
+ case ast.Constant(value=value):
+ return value is not None
+ case ast.Name(id=name):
+ return name in params and name in self._non_null
+ case ast.Compare(ops=[op]):
+ # is/is not lower to isNull/isNotNull; ==/!= cover NULL with
boolean
+ # literals; ordering raises on NULL rather than returning it.
+ return isinstance(
+ op, (ast.Is, ast.IsNot, ast.Eq, ast.NotEq, ast.Lt,
ast.LtE, ast.Gt, ast.GtE)
+ )
+ case ast.UnaryOp(op=ast.Not()):
+ # Either inverts a non-NULL operand or coalesces against a
literal.
+ return True
+ case ast.UnaryOp(operand=operand):
+ return self._is_never_null(params, operand)
+ case ast.BoolOp(values=values):
+ return all(self._is_never_null(params, v) for v in values)
+ case ast.BinOp(left=left, right=right):
+ # Catalyst arithmetic and concat propagate NULL from either
side.
+ return self._is_never_null(params, left) and
self._is_never_null(params, right)
+ case ast.Call(func=ast.Name(id="len"), args=[_], keywords=[]):
+ # The lowering raises on NULL or returns ``length``; neither
is NULL,
+ # so a parent (``len(x) > 0``) must not add a second check. A
non-string
+ # argument never produces a column -- that variant is dropped
first.
+ return True
+ case ast.IfExp(test=test, body=body, orelse=orelse):
+ # Each branch is lowered under the facts its test proves, so
ask
+ # about the branches the same way.
+ when_true, when_false = _null_facts(test)
+ with self._narrowed(when_true):
+ body_ok = self._is_never_null(params, body)
+ with self._narrowed(when_false):
+ return body_ok and self._is_never_null(params, orelse)
+ case _:
+ return False
+
+ @staticmethod
+ def _any_null(columns: List[Column]) -> Column:
+ """``columns[0] IS NULL OR ...``; raises on an empty list."""
+ return functools.reduce(operator.or_, (column.isNull() for column in
columns))
+
+ def _raise_on_null(
+ self,
+ params: List[str],
+ operands: List[Tuple[ast.AST, Column]],
+ label: str,
+ message: str,
+ otherwise: Column,
+ ) -> Column:
+ """``otherwise``, guarded so that a NULL operand raises ``message``
instead.
+
+ The only place that emits a ``raise_error``; future lowerings needing
one
+ (SPARK-55210's arithmetic/unary/concat guards) must come through here
or they
+ skip the narrowing and the warning. Operands proven non-NULL
contribute no
+ check, and with none left the guard is dropped -- ``RaiseError`` is
+ ``throwable`` (SPARK-58627), so a plan holding one cannot be pushed
through a
+ join. ``label`` names the construct plus the parameters the emitted
+ ``isNull`` calls actually test -- ``a is not None and a + b > 0`` names
+ ``b`` alone.
+ """
+ checked = [(node, c) for node, c in operands if not
self._is_never_null(params, node)]
+ if not checked:
+ return otherwise
+ named = sorted({name for node, _ in checked for name in
self._checked_names(params, node)})
+ self._pending_null_guards |= {f"{label} on {', '.join(named)}" if
named else label}
+ guard = self._any_null([c for _, c in checked])
+ return when(guard, raise_error(lit(message))).otherwise(otherwise)
+
+ def _checked_names(self, params: List[str], node: ast.AST) -> set:
+ """The parameters in ``node`` whose possible NULL-ness a guard checks.
+
+ The walk prunes whatever ``_is_never_null`` proves: a parameter an
+ enclosing guard already established (``a is not None and a + b > 0``
+ warns about ``b``, not ``a``) and a sub-expression carrying its own
+ check (``len(...)`` never returns NULL and has its own label), so the
+ label points at the guard the user is still missing.
+ """
+ if self._is_never_null(params, node):
+ return set()
+ if isinstance(node, ast.Name):
+ return {node.id} if node.id in params else set()
+ return {
+ name
+ for child in ast.iter_child_nodes(node)
Review Comment:
**Nit (P3):** The new pruning handles guards that enclose the operand, but
not a guard inside it. When `_checked_names` reaches a ternary, it recurses
into the children under the caller's facts only, so it skips the per-arm
narrowing that `_is_never_null` applies through `_null_facts(test)`.
Take `lambda x, y: len(x if x is not None else y)` on string columns under
LongType. The check correctly survives, because a NULL `y` can still reach
`len`. But the label comes out as "`len` on x, y", so the warning asks the user
to guard `x`, which the ternary already does. A parameter that appears only in
the test is named as well, for example `a` in `len(b if a else c)`. Calling
`CatalystTranspiler()._checked_names(['x', 'y'], <x if x is not None else y>)`
on this head returns `['x', 'y']`.
Handling `ast.IfExp` the way `_is_never_null` does would fix it: skip the
test, walk `body` under `self._narrowed(when_true)`, and walk `orelse` under
`self._narrowed(when_false)`. A ternary-operand case in
`test_udf_transpile_warns_only_about_the_unproven_parameter` would pin it.
**Verification:**
- **Regression:** A UDF whose `len` or comparison operand is a
coalesce-style ternary over two parameters warns about only the else-arm
parameter and does not name the parameter the ternary already guards.
- **Behavior:** The existing warning-label expectations for mixed,
unguarded, and borrowed-`len` operands still hold.
- **Inspection:** The lowered option for the ternary case still carries
exactly one raising check, confirming that only the label changed.
##########
python/pyspark/sql/transpile.py:
##########
@@ -213,6 +229,80 @@ def _is_definitely_boolean(node: ast.AST) -> bool:
return False
+def _none_check_operand(left: ast.AST, comparator: ast.AST) ->
Optional[ast.AST]:
+ """The non-``None`` side of an ``x is None`` / ``None is x`` pair, else
``None``.
+
+ Shared by ``_convert_chunk``'s lowering and ``_null_facts``, which must
agree on
+ exactly which shapes count: a comparison against anything but the literal
``None``
+ is an object-identity test with no SQL equivalent.
+ """
+ is_none_left = isinstance(left, ast.Constant) and left.value is None
+ is_none_right = isinstance(comparator, ast.Constant) and comparator.value
is None
+ if not (is_none_left or is_none_right):
+ return None
+ return comparator if is_none_left else left
+
+
+def _none_check_subject(node: ast.AST) -> Optional[str]:
+ """The parameter name in an ``x is None`` / ``x == None`` test, or
``None``.
+
+ Either operand order, and the ``==``/``!=`` spellings too. Only a bare
name is
+ reported -- that is all the nullability environment can key on.
+ """
+ if not isinstance(node, ast.Compare) or len(node.ops) != 1 or
len(node.comparators) != 1:
+ return None
+ if not isinstance(node.ops[0], (ast.Is, ast.IsNot, ast.Eq, ast.NotEq)):
+ return None
+ subject = _none_check_operand(node.left, node.comparators[0])
+ return subject.id if isinstance(subject, ast.Name) else None
+
+
+def _null_facts(node: ast.AST) -> Tuple[frozenset, frozenset]:
+ """The parameters ``node`` proves non-NULL, as ``(when_true, when_false)``.
+
+ * ``x is not None`` / ``x != None`` prove ``x`` when true; ``is None`` /
``==``
+ when false.
+ * ``not A`` swaps ``A``'s outcomes.
+ * ``A and B`` true means both held, so their true-facts combine; false
says only
+ that one failed, so it proves nothing. ``A or B`` is the mirror image.
+ * Anything else proves nothing, which is the safe direction -- a fact we
miss
+ leaves a check in place, it never drops one we needed.
+
+ Callers use "not FALSE => true-facts, not TRUE => false-facts" rather than
just
+ "TRUE => true-facts", because ``And``/``Or`` and ``CASE WHEN`` treat NULL
as
+ not-true, not false -- and a node a fact came FROM cannot itself be NULL.
+
+ TODO (SPARK-55218): with multi-statement bodies, an ``if x is None: return
...``
+ should add its false-facts to the statements after it.
+ """
+ empty: frozenset = frozenset()
+ # ``_none_check_subject`` takes ``==``/``!=`` too, or `if x != None:`
would keep
+ # a check and then be told to add the guard it already had.
+ if isinstance(node, ast.Compare) and (subject :=
_none_check_subject(node)) is not None:
+ proven = frozenset({subject})
+ # `x is None` / `x == None` prove nothing when true and non-NULL-ness
+ # when false; `is not` / `!=` are the other way round.
+ if isinstance(node.ops[0], (ast.Is, ast.Eq)):
+ return empty, proven
+ return proven, empty
+ match node:
+ case ast.UnaryOp(op=ast.Not(), operand=operand):
+ when_true, when_false = _null_facts(operand)
+ return when_false, when_true
+ case ast.BoolOp(op=ast.And(), values=values):
+ proven = empty
+ for value in values:
+ proven |= _null_facts(value)[0]
+ return proven, empty
+ case ast.BoolOp(op=ast.Or(), values=values):
+ proven = empty
+ for value in values:
+ proven |= _null_facts(value)[1]
+ return empty, proven
+ case _:
Review Comment:
**Nit (P3):** A bare truthiness test proves its parameter non-NULL in the
branch where it held, but this catch-all gives it no facts. `_convert_if_like`
lowers `if s:` through `_truthiness_col`, which coalesces NULL to False, so the
body only ever runs for a non-NULL `s`.
Still, in `def f(s): if s: return len(s) else: return 0` (a string column
under LongType, using the existing bare-`if` truthiness lowering), `len(s)`
keeps an unreachable `raise_error`. That is the same throwable guard this PR
sets out to remove. The new warning then says the UDF "still checks for NULL in
`len` on s" and suggests `if x is not None:`, although `if s:` already excludes
None. Acting on that advice is awkward too, because `s is not None and s` is
refused as a non-boolean `and` operand.
A `case ast.Name(id=name): return frozenset({name}), empty` arm looks sound
for every consumer:
- An if/ternary truthiness test is coalesced to False.
- `and`/`or` refuse bare-name operands before narrowing.
- `not` on a bool-category name swaps the fact to the false side, where it
holds because `coalesce(~b, True)` is FALSE only when `b` is TRUE.
If you'd rather not widen the facts, narrowing the module docstring and the
warning text to `is None`-style tests would at least keep them accurate.
**Verification:**
- **Regression:** A string UDF guarded by `if s:` around `len(s)` has no
raise_error in its optimized plan, returns Python's values for empty,
non-empty, and None inputs, and does not emit the surviving-check warning.
- **Behavior:** When the check sits in the arm the truthiness test does not
prove, the check survives and a None row raises like Python.
- **Behavior:** The fact table reports truthiness facts for a bare name and
its negation, and all existing fact-table expectations still hold.
##########
python/pyspark/sql/tests/test_udf_transpile_unit.py:
##########
@@ -2634,6 +2663,529 @@ def bytes_ident(x: bytes):
self.assertFalse(UserDefinedFunction(bool_add,
LongType()).transpiled)
self.assertTrue(UserDefinedFunction(bytes_ident,
BinaryType()).transpiled)
+ def _transpiled_plan(self, func, return_type, schema):
+ """The optimized plan of ``func`` applied to every column of
``schema``.
+
+ The lowered NULL checks are what these tests are about, so they have
to be
+ read off the plan: the values a guarded UDF returns are the same
whether or
+ not the plan carries a check that can never fire.
+
+ Asserting the option was actually APPLIED is what keeps every
+ ``assertNotIn(..., plan)`` built on this from passing for the wrong
reason
+ -- an interpreted plan contains none of the strings we look for, so a
UDF
+ that quietly fell back would satisfy them all (see ``_vals``, which
guards
+ the same way for the same reason).
+ """
+ with self.sql_conf(_TRANSPILE_ON):
+ u = self._transpiled_udf(func, return_type)
+ df = self.spark.createDataFrame([], schema)
+ projected = df.select(u(*df.columns))
+ self.assertEqual(0, self._eval_python_count(projected), str(func))
+ return projected._jdf.queryExecution().optimizedPlan().toString()
+
+ def test_udf_transpile_drops_null_checks_a_guard_already_made(self):
+ # SPARK-58628. Python raises TypeError on `None > 0` where Spark
returns
+ # NULL, so an ordering comparison carries a check that raises. When
the UDF
+ # has already tested the parameter the check can never fire, and
leaving it
+ # in is not free: RaiseError is throwable (SPARK-58627), which stops
the
+ # optimizer from moving a predicate containing it. Every shape below
proves
+ # non-NULL-ness, so none of them should lower to a raise -- and all of
them
+ # must still agree with Python, including on the None row.
+ def if_guard(x):
+ if x is not None:
+ return x > 0
+ else:
+ return None
+
+ def else_of_is_none(x):
+ if x is None:
+ return None
+ else:
+ return x > 0
+
+ def not_is_none(x):
+ if not (x is None): # noqa: E714 the `not` form is the point
+ return x > 0
+ else:
+ return None
+
+ def reversed_test(x):
+ if None is not x:
+ return x > 0
+ else:
+ return None
+
+ def ne_none_test(x):
+ # The `!= None` spelling of the same guard. PEP 8 prefers `is not
None`,
+ # but both are common and both must narrow, or the equality form
keeps a
+ # raising check and gets told to add the guard it already has.
+ if x != None: # noqa: E711
+ return x > 0
+ else:
+ return None
+
+ def eq_none_test(x):
+ if x == None: # noqa: E711
+ return None
+ else:
+ return x > 0
+
+ ternary = lambda x: (x > 0) if x is not None else None # noqa: E731
+ short_circuit_and = lambda x: x is not None and x > 0 # noqa: E731
+ short_circuit_or = lambda x: x is None or x > 0 # noqa: E731
+ # `<=` / `>=` share `_lower_value_compare` but are otherwise only
exercised
+ # where a guard IS emitted, so cover the narrowed side for them too.
+ lte_guarded = lambda x: (x <= 0) if x is not None else None # noqa:
E731
+ gte_guarded = lambda x: (x >= 0) if x is not None else None # noqa:
E731
+ lt_guarded = lambda x: (x < 0) if x is not None else None # noqa: E731
+
+ rows = [(-1,), (0,), (5,), (None,)]
+ cases = [
+ (if_guard, [False, False, True, None]),
+ (else_of_is_none, [False, False, True, None]),
+ (not_is_none, [False, False, True, None]),
+ (reversed_test, [False, False, True, None]),
+ (ne_none_test, [False, False, True, None]),
+ (eq_none_test, [False, False, True, None]),
+ (ternary, [False, False, True, None]),
+ (lte_guarded, [True, True, False, None]),
+ (gte_guarded, [False, True, True, None]),
+ (lt_guarded, [True, False, False, None]),
+ # `and` / `or` return the guard's own result for None, as Python
does.
+ (short_circuit_and, [False, False, True, False]),
+ (short_circuit_or, [False, False, True, True]),
+ ]
+ for func, expected in cases:
+ with self.subTest(func=getattr(func, "__name__", "lambda")):
+ self.assertEqual(
+ expected,
+ [func(v) for (v,) in rows],
+ "the expectation should be what Python actually does",
+ )
+ self.assertEqual(expected, self._vals(func, BooleanType(), "a
long", rows))
+ plan = self._transpiled_plan(func, BooleanType(), "a long")
+ self.assertNotIn("raise_error", plan)
+
+ def test_udf_transpile_keeps_the_null_check_it_needs(self):
+ # The other side of the previous test: nothing here proves the
parameter
+ # non-NULL, so the check has to stay and the UDF has to raise like
Python.
+ # `x is None and ...` is the interesting one -- an `and` being false
says
+ # only that SOME operand was false, so it narrows nothing.
+ unguarded = lambda x: x > 0 # noqa: E731
+ wrong_way_and = lambda x: x is None and x > 0 # noqa: E731
+ two_columns = lambda a, b: a > b # noqa: E731
+
+ for func, schema in ((unguarded, "a long"), (two_columns, "a long, b
long")):
+ with self.subTest(func="lambda", schema=schema):
+ plan = self._transpiled_plan(func, BooleanType(), schema)
+ self.assertIn("raise_error", plan)
+ boolean = BooleanType()
+ self._raises(unguarded, "a long", [(None,)], "cannot compare null",
boolean)
+ self._raises(two_columns, "a long, b long", [(1, None)], "cannot
compare null", boolean)
+ # Each ordering operator names itself in the guard's message and in the
+ # warning's label; only `>` was covered before.
+ lt_zero = lambda x: x < 0 # noqa: E731
+ lte_zero = lambda x: x <= 0 # noqa: E731
+ gte_zero = lambda x: x >= 0 # noqa: E731
+ for func, op_text in ((lt_zero, "`<`"), (lte_zero, "`<=`"), (gte_zero,
"`>=`")):
+ with self.subTest(op=op_text):
+ with self.sql_conf(_TRANSPILE_ON):
+ _, warned = self._udf_and_warnings(func, boolean)
+ self.assertIn(f"comparison {op_text} on x", warned)
+ self._raises(func, "a long", [(None,)], f"operator {op_text}",
boolean)
+ # `x is None and x > 0` is False for every non-None x without
evaluating the
+ # comparison, and raises for None -- exactly like Python.
+ with self.sql_conf(_TRANSPILE_ON):
+ u = self._transpiled_udf(wrong_way_and, BooleanType())
+ self.assertIn("raise_error", str(u.transpiled[0]))
+ df = self.spark.createDataFrame([(1,)], "a long")
+ self.assertEqual([False], [r[0] for r in
df.select(u("a")).collect()])
+ self._raises(wrong_way_and, "a long", [(None,)], "cannot compare
null", boolean)
+
+ def test_udf_transpile_narrows_only_the_proven_parameter(self):
+ # With two parameters the narrowing has to be per-name: guarding `a`
must
+ # not license dropping the check on `b`. Both guarded, no check at all.
+ one_guarded = lambda a, b: a is not None and a > b # noqa: E731
+ both_guarded = lambda a, b: a is not None and b is not None and a > b
# noqa: E731
+
+ # The same two-parameter guards written as an `if` TEST rather than as
the
+ # expression itself. This is the only shape that asks `_null_facts`
about a
+ # composite `and` / `or` node instead of about a single comparison,
and it
+ # is the idiomatic way to guard two columns.
+ def composite_and(a, b):
+ if a is not None and b is not None:
+ return a > b
+ else:
+ return None
+
+ def composite_or(a, b):
+ if a is None or b is None:
+ return None
+ else:
+ return a > b
+
+ def composite_not_or(a, b):
+ if not (a is None or b is None):
+ return a > b
+ else:
+ return None
+
+ schema = "a long, b long"
+ partial = self._transpiled_plan(one_guarded, BooleanType(), schema)
+ self.assertIn("raise_error", partial)
+ # Only `b` is still checked. Note `isnull(a` does not match the
guard's own
+ # `isnotnull(a...)`, which is the test rather than a check on the
operand.
+ self.assertIn("isnull(b", partial)
+ self.assertNotIn("isnull(a", partial)
+ rows = [(5, 1), (1, 5), (1, None), (None, 1)]
+ for func in (both_guarded, composite_and, composite_or,
composite_not_or):
+ with self.subTest(func=getattr(func, "__name__", "lambda")):
+ self.assertNotIn("raise_error", self._transpiled_plan(func,
BooleanType(), schema))
+ self.assertEqual(
+ [func(a, b) for a, b in rows],
+ self._vals(func, BooleanType(), schema, rows),
+ )
+ # Partial narrowing was only plan-asserted, but it creates two runtime
+ # invariants: the surviving check must still fire for a NULL `b`, and
the
+ # guard on `a` must still short-circuit a NULL `a` to False without
reaching
+ # the comparison. This is the one live consumer of the claim that
Catalyst's
+ # `And` short-circuits on false the way Python does.
+ self._raises(one_guarded, schema, [(1, None)], "cannot compare null",
BooleanType())
+ self.assertEqual([False], self._vals(one_guarded, BooleanType(),
schema, [(None, None)]))
+ self.assertEqual([False], self._vals(one_guarded, BooleanType(),
schema, [(None, 1)]))
+
+ # The UNSOUND direction, end to end: a true `or` says only that ONE
operand
+ # held, so it narrows nothing and the check must stay. Covered here as
well as
+ # in `test_null_facts_narrow_by_outcome`, because a plausible
"symmetry" edit
+ # to `_null_facts` would delete a needed raise, and the fact-table
unit test
+ # sits one remove from the plan that would go wrong.
+ def or_test(a, b):
+ if a is not None or b is not None:
+ return a > 0
+ else:
+ return None
+
+ def not_and_test(a, b):
+ if not (a is None and b is None):
+ return a > 0
+ else:
+ return None
+
+ for func in (or_test, not_and_test):
+ with self.subTest(func=func.__name__):
+ self.assertIn("raise_error", self._transpiled_plan(func,
BooleanType(), schema))
+ with self.assertRaises(Exception):
+ func(None, 1)
+ self._raises(func, schema, [(None, 1)], "cannot compare null",
BooleanType())
+
+ def test_udf_transpile_narrowing_never_drops_a_needed_check(self):
+ # The unsafe direction of the narrowing paths: dropping a check that is
+ # still needed turns Python's TypeError into a silent NULL or False.
+ # Each case keeps its raise on the unproven side, next to the guarded
+ # counterpart that earns the elision.
+ boolean = BooleanType()
+ # A comparison BEFORE its guard: `and` narrows an operand only by the
+ # operands before it, so `x > 0` here knows nothing and must still
raise
+ # for None, as Python does. Narrowing by LATER operands would drop this
+ # check and quietly return False for None.
+ late_guard = lambda x: x > 0 and x is not None # noqa: E731
+ self.assertIn("raise_error", self._transpiled_plan(late_guard,
boolean, "a long"))
+ self._raises(late_guard, "a long", [(None,)], "cannot compare null",
boolean)
+ self.assertEqual([True, False], self._vals(late_guard, boolean, "a
long", [(1,), (-1,)]))
+
+ # An arithmetic operand is NULL when either side is, so `x + 1` is
+ # checked until `x` is proven: unguarded it raises like Python, guarded
+ # it loses the check entirely. An `or` in `_is_never_null`'s BinOp arm
+ # would call `x + 1` safe on the strength of the literal alone and
+ # return NULL for a None row.
+ arith = lambda x: x + 1 > 0 # noqa: E731
+ arith_guarded = lambda x: x is not None and x + 1 > 0 # noqa: E731
+ self.assertIn("raise_error", self._transpiled_plan(arith, boolean, "a
long"))
+ self._raises(arith, "a long", [(None,)], "cannot compare null",
boolean)
+ self.assertNotIn("raise_error", self._transpiled_plan(arith_guarded,
boolean, "a long"))
+ rows = [(1,), (-5,), (None,)]
+ self.assertEqual([True, False, False], self._vals(arith_guarded,
boolean, "a long", rows))
+
+ # A ternary narrows each arm by its OWN outcome: the body knows `x`,
+ # the else does not, so exactly one check survives -- the else's
+ # `y > 0`. Swapping the branch facts checks `x` in the body too (two
+ # raise_error), and the (None, None) row would stop raising.
Review Comment:
**Nit (P3):** The second half of this claim doesn't hold. If the branch
facts were swapped, the body `x > 0` would be lowered under the empty
false-facts and gain a check on `x`. The else arm `y > 0` would be lowered
under {x}, which says nothing about `y`, so its check stays. A (None, None) row
takes the else arm and still raises.
So under a swap, the `_raises(branches, ..., [(None, None)], ...)` call
below and the warning-label assertion both still pass. Only `assertEqual(1,
plan.count("raise_error"), plan)` catches it. Wording like "Swapping the branch
facts adds a second raise_error (caught by the count below); the (None, None)
row still raises through the else arm's own check on `y`" would match what the
test actually pins.
**Verification:**
- **Inspection:** The comment's account of which assertion detects a
swapped-facts regression matches the lowering: the else arm keeps its check on
`y`, and only the raise_error count changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
