Configuration for Division By Zero Behavior by hadia206 · Pull Request #470 · bodo-ai/PyDough

hadia206 · 2026-01-09T00:39:19Z

Adds configurable division-by-zero behavior via PyDoughConfigs.division_by_zero.
3 options:
- DATABASE: Let database handle it (default)
- NULL: a / b will be a / KEEP_IF(b, b != 0)
- ZERO: a / b will be IFF(b == 0, 0, a / b)
Conversions happen before simplification so the simplifier can optimize out guards when possible.

Closes #213

…division_by_zero_config

john-sanchez31

Looks good. Great job Hadia. Couple of comments below related to type hints and the final sql and more suggested tests

john-sanchez31 · 2026-01-27T22:18:04Z

pydough/conversion/relational_simplification.py

+                        ],
+                    )
+                    # b != 0 (used to mark divisor as processed)
+                    is_not_zero_expr = CallExpression(


check type hints

It's already in the one under case DivisionByZeroBehavior.NULL:. PyPi doesn't allowed repeated type hints in this case scenario.

john-sanchez31 · 2026-01-27T22:24:10Z

tests/test_sql_refsols/division_by_zero_zero_mysql.sql

@@ -0,0 +1,6 @@
+SELECT
+  CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END AS computed_value


Is it necessary the NULLIF(l_discount, 0) part, since we already know at that point that l_discount != 0. Same for other dialects

We don't know that it's not 0
That requires knowing the actual data stored in the column.

This l_discount actually has 0 which is why I chose it for the test and the result of the tests shows output to indicate that

I didn't get it. If l_discount is zero then it should do the THEN 0 part right?
If it is still zero then l_extendedprice / NULLIF(l_discount, 0) becomes l_extendedprice / NULL that becomes NULL instead of zero. Am I wrong?

Yes, If l_discount = 0 it'll go into the THEN 0 so it won't enter into the ELSE so the NULLIF(l_discount, 0) won't enter since it's in the ELSE.

john-sanchez31 · 2026-01-27T22:26:13Z

tests/test_session.py

+        pytest.param(DivisionByZeroBehavior.ZERO, id="zero"),
+    ],
+)
+def test_division_by_zero_e2e(


Do we need type hints for this tests? Not sure, just in case a reminder

Thanks. Most are clear but added to some anyway.

john-sanchez31 · 2026-01-27T22:37:19Z

tests/test_pydough_functions/simple_pydough_functions.py

+def simple_division_by_zero():
+    return lines.CALCULATE(computed_value=extended_price / discount).TOP_K(
+        1, by=computed_value.ASC(na_pos="first")
+    )


Can we add more tests? For example

With multiple operations like a*(c / b) + z

Operations inside the division like (a+b)/ (z*2)

And things with KEEP_IF/IFF already in the denominator:

x / KEEP_IF(y, y < 10)

x / KEEP_IF(y, y > 10)

x / IFF(y > 0, y, 1)

x / IFF(y > 0, 1, y)

knassre-bodo · 2026-01-28T18:19:48Z

pydough/configs/pydough_configs.py

+    """
+
+    DATABASE = "DATABASE"
+    """Leave it alone and let the database resolve it."""


Let's be more specific than "leave it alone"

knassre-bodo · 2026-01-28T18:21:14Z

pydough/configs/pydough_configs.py

                return 5


+class DivisionByZeroBehavior(Enum):


Let's set the values in default_config and defog_config in conftest.py

knassre-bodo · 2026-01-28T18:25:03Z

pydough/conversion/relational_simplification.py

+            if self.strip_zero_guards(divisor) is not divisor:
+                return expr, arg_predicates


This seems problematic in other situations where there is a KEEP_IF / IFF in the denominator for unrelated reasons. E.g. x / KEEP_IF(y, y < 10)

Good point. I'll update the code to check only for our initial replacement (zero check)

knassre-bodo · 2026-01-28T18:25:32Z

pydough/conversion/relational_simplification.py

        arg_predicates: list[PredicateSet] = [
            self.stack.pop() for _ in range(len(new_call.inputs))
        ]
+        new_call, arg_predicates = self.update_div_expr(new_call, arg_predicates)


I think the insertion of the logic for the KEEP_IF / IFF should be in the relational conversion, rather than the simplifier, since the simplifier can be run more than once. Plus, the simplifier can delete the KEEP_IF / IFF calls if they are redundant (e.g. we know the data is going to be non-zero)

That would be in translate_expression in relational_converter.py (would need to pass in the session to the constructor for the translator so you could access self.session inside translate_expression).

We could move all of this logic to that file as a helper function called at the right time (minus the part that I highlighted as a potential problem). Also, I'd recommend generalizing it slightly so that we can trivially extend this rewrite to apply to MOD as well as DIV, once we implement MOD (e.g. make the rewrite steps use expr.op instead of pydop.DIV)

knassre-bodo · 2026-01-28T18:27:09Z

tests/test_session.py

+        case "snowflake":
+            sf_conn = request.getfixturevalue("sf_conn_db_context")
+            return (
+                sf_conn("SNOWFLAKE_SAMPLE_DATA", "TPCH_SF1"),
+                get_sf_sample_graph("TPCH"),
+            )


Ok, this is extremely clever!!!

knassre-bodo · 2026-01-28T18:27:59Z

tests/test_session.py

+
+@pytest.fixture(
+    params=[
+        pytest.param("sqlite", id="sqlite"),


Why do all of them have execute except this one? Realistically I don't think any of them need it?

Good catch. updated it

knassre-bodo · 2026-01-28T18:28:31Z

tests/test_session.py

+    request,
+    get_sample_graph: graph_fetcher,
+    get_sf_sample_graph: graph_fetcher,
+) -> tuple[DatabaseContext, GraphMetadata]:


I think we may want to make this a more general fixture in conftest.py that doesn't have to do with division by zero, it is just about getting the TPCH graph/db for each database. With that, we could gradually merge some of our tests for the different dialects that are currently split up across multiple files.

knassre-bodo · 2026-01-28T18:38:30Z

tests/test_pydough_functions/simple_pydough_functions.py

+def simple_division_by_zero():
+    return lines.CALCULATE(computed_value=extended_price / discount).TOP_K(
+        1, by=computed_value.ASC(na_pos="first")
+    )


And things with KEEP_IF/IFF already in the denominator:

x / KEEP_IF(y, y < 10)

x / KEEP_IF(y, y > 10)

x / IFF(y > 0, y, 1)

x / IFF(y > 0, 1, y)

john-sanchez31

LGTM!

juankx-bodo · 2026-02-02T23:57:43Z

tests/test_sql_refsols/division_by_zero_zero_mysql.sql

@@ -0,0 +1,6 @@
+SELECT
+  CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END AS computed_value


I didn't get it. If l_discount is zero then it should do the THEN 0 part right?
If it is still zero then l_extendedprice / NULLIF(l_discount, 0) becomes l_extendedprice / NULL that becomes NULL instead of zero. Am I wrong?

juankx-bodo · 2026-02-03T00:06:04Z

tests/test_session.py

+
+    # DATABASE mode: Snowflake/Postgres throw errors, SQLite/MySQL return NULL
+    if division_by_zero_config == DivisionByZeroBehavior.DATABASE:
+        if db_context.dialect in (DatabaseDialect.SNOWFLAKE, DatabaseDialect.POSTGRES):


I think snowflake default behavior to division by zero is return NULL, not error.

No, default behavior for Snowflake is error. Please see attached screenshot example. Plus, the test won't pass if it's not giving an error :-)

knassre-bodo

A few lingering matters to address, but close to being ready to merge :)

knassre-bodo · 2026-02-05T18:15:11Z

pydough/conversion/relational_converter.py

+
+            case DivisionByZeroBehavior.ZERO:
+                # If b == 0, return 0
+                # Replace with IFF(b == 0, 0, a / guarded_b)


Suggested change

# Replace with IFF(b == 0, 0, a / guarded_b)

# Replace with IFF(b == 0, 0, a / b)

knassre-bodo · 2026-02-05T18:16:41Z

pydough/conversion/relational_converter.py

+        is_rhs_zero = isinstance(rhs, LiteralExpression) and rhs.value == 0
+        return (lhs == value and is_rhs_zero) or (rhs == value and is_lhs_zero)
+
+    def _strip_zero_guards(self, expr: RelationalExpression) -> RelationalExpression:


Why not just have this return True/False for if the denominator is already zero-guarded?

knassre-bodo · 2026-02-05T18:17:47Z

pydough/conversion/relational_converter.py

+                # Only strip if condition is 'then_value != 0'
+                condition = expr.inputs[0]
+                then_value = expr.inputs[1]
+                if self._is_zero_check_condition(condition, then_value):


This should only be removed when doing IFF(x != 0, x, 0).

E.g. what if it is IFF(x != 0, x, 3)?

knassre-bodo · 2026-02-05T18:24:27Z

tests/conftest.py

    config.avg_default_zero = False
    config.start_of_week = DayOfWeek.MONDAY
    config.start_week_as_zero = True
+    config.division_by_zero = DivisionByZeroBehavior.DATABASE


I think for defog, we may actually want it to be NULL based on how a lot of the queries are written. Let's see if this is correct or if it breaks everything. if it is correct, lets go back to a lot of our existing PyDough code for defog queries and remove a lot of the existing division guards/checks (e.g. impl_defog_restaurants_gen13)

knassre-bodo · 2026-02-05T18:25:58Z

tests/test_sql_refsols/division_by_zero_zero_ansi.sql

+SELECT
+  CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END AS computed_value


Why is there a NULLIF here as well? It should just be CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / l_discount. This indicates that the denominator is being affected twice?

knassre-bodo · 2026-02-05T18:27:43Z

tests/test_session.py

+        # IFF in denominator - divisor in false branch
+        pytest.param(
+            (
+                DivisionByZeroBehavior.ZERO,


Also try these weird variants with the NULL behavior.

knassre-bodo · 2026-02-05T18:28:18Z

tests/test_sql_refsols/division_multiple_ops_zero_mysql.sql

@@ -0,0 +1,6 @@
+SELECT
+  l_quantity * CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END + l_tax AS computed_value


This does not appear to be how the conversion step is supposed to work?

hadia206 added 6 commits January 8, 2026 16:38

division by zero config

2260e73

update test and code

86f514b

[run all] pin connector version

f275a6d

[run all] fix test

77b32b0

Merge branch 'main' of https://github.com/bodo-ai/PyDough into Hadia/…

7e153b2

…division_by_zero_config

[run all]

ed24c55

hadia206 marked this pull request as ready for review January 23, 2026 18:19

hadia206 requested review from a team, john-sanchez31, juankx-bodo and knassre-bodo and removed request for a team January 23, 2026 18:19

john-sanchez31 reviewed Jan 27, 2026

View reviewed changes

knassre-bodo reviewed Jan 28, 2026

View reviewed changes

[run CI][run SF][run mysql][run postgres] address comments

0e989f9

hadia206 requested review from john-sanchez31 and knassre-bodo January 29, 2026 21:47

john-sanchez31 approved these changes Feb 2, 2026

View reviewed changes

juankx-bodo reviewed Feb 3, 2026

View reviewed changes

knassre-bodo reviewed Feb 5, 2026

View reviewed changes

		@@ -0,0 +1,6 @@
		SELECT
		CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END AS computed_value

		if self.strip_zero_guards(divisor) is not divisor:
		return expr, arg_predicates

	# Replace with IFF(b == 0, 0, a / guarded_b)
	# Replace with IFF(b == 0, 0, a / b)

		@@ -0,0 +1,6 @@
		SELECT
		l_quantity * CASE WHEN l_discount = 0 THEN 0 ELSE l_extendedprice / NULLIF(l_discount, 0) END + l_tax AS computed_value

Conversation

hadia206 commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-sanchez31 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hadia206 Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-sanchez31 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juankx-bodo Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knassre-bodo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

hadia206 commented Jan 9, 2026 •

edited

Loading

hadia206 Jan 28, 2026 •

edited

Loading

juankx-bodo Feb 3, 2026 •

edited

Loading