Skip to content

Commit 8bd2483

Browse files
committed
fix(macros): keep SAFE_ADD, SAFE_SUB and SAFE_DIV grouped inside surrounding operators
The optimizer collapses the CASE built by SAFE_ADD/SAFE_SUB into its ELSE branch without parentheses, so an enclosing operator swallows an operand. Emit the ELSE arithmetic as an explicit Paren. SAFE_DIV now returns its quotient wrapped in a Paren so x / @SAFE_DIV(a, b) keeps its grouping. Closes #5649 Signed-off-by: Rodrigo-Palma <email.rodrigopalma@gmail.com>
1 parent ad2377e commit 8bd2483

3 files changed

Lines changed: 75 additions & 13 deletions

File tree

docs/concepts/macros/sqlmesh_macros.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -887,7 +887,7 @@ would be rendered as:
887887

888888
```sql linenums="1"
889889
SELECT
890-
CASE WHEN a IS NULL AND b IS NULL AND c IS NULL THEN NULL ELSE COALESCE(a, 0) + COALESCE(b, 0) + COALESCE(c, 0) END
890+
CASE WHEN a IS NULL AND b IS NULL AND c IS NULL THEN NULL ELSE (COALESCE(a, 0) + COALESCE(b, 0) + COALESCE(c, 0)) END
891891
FROM foo
892892
```
893893

@@ -906,7 +906,7 @@ would be rendered as:
906906

907907
```sql linenums="1"
908908
SELECT
909-
CASE WHEN a IS NULL AND b IS NULL AND c IS NULL THEN NULL ELSE COALESCE(a, 0) - COALESCE(b, 0) - COALESCE(c, 0) END
909+
CASE WHEN a IS NULL AND b IS NULL AND c IS NULL THEN NULL ELSE (COALESCE(a, 0) - COALESCE(b, 0) - COALESCE(c, 0)) END
910910
FROM foo
911911
```
912912

@@ -925,7 +925,7 @@ would be rendered as:
925925

926926
```sql linenums="1"
927927
SELECT
928-
a / NULLIF(b, 0)
928+
(a / NULLIF(b, 0))
929929
FROM foo
930930
```
931931

sqlmesh/core/macros.py

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,12 +1052,11 @@ def safe_add(_: MacroEvaluator, *fields: exp.Expr) -> exp.Case:
10521052
>>> from sqlmesh.core.macros import MacroEvaluator
10531053
>>> sql = "SELECT @SAFE_ADD(a, b) FROM foo"
10541054
>>> MacroEvaluator().transform(parse_one(sql)).sql()
1055-
'SELECT CASE WHEN a IS NULL AND b IS NULL THEN NULL ELSE COALESCE(a, 0) + COALESCE(b, 0) END FROM foo'
1055+
'SELECT CASE WHEN a IS NULL AND b IS NULL THEN NULL ELSE (COALESCE(a, 0) + COALESCE(b, 0)) END FROM foo'
10561056
"""
1057-
return (
1058-
exp.Case()
1059-
.when(exp.and_(*(field.is_(exp.null()) for field in fields)), exp.null())
1060-
.else_(reduce(lambda a, b: a + b, [exp.func("COALESCE", field, 0) for field in fields])) # type: ignore
1057+
return _null_if_all_null(
1058+
fields,
1059+
reduce(lambda a, b: a + b, [exp.func("COALESCE", field, 0) for field in fields]), # type: ignore
10611060
)
10621061

10631062

@@ -1070,27 +1069,41 @@ def safe_sub(_: MacroEvaluator, *fields: exp.Expr) -> exp.Case:
10701069
>>> from sqlmesh.core.macros import MacroEvaluator
10711070
>>> sql = "SELECT @SAFE_SUB(a, b) FROM foo"
10721071
>>> MacroEvaluator().transform(parse_one(sql)).sql()
1073-
'SELECT CASE WHEN a IS NULL AND b IS NULL THEN NULL ELSE COALESCE(a, 0) - COALESCE(b, 0) END FROM foo'
1072+
'SELECT CASE WHEN a IS NULL AND b IS NULL THEN NULL ELSE (COALESCE(a, 0) - COALESCE(b, 0)) END FROM foo'
1073+
"""
1074+
return _null_if_all_null(
1075+
fields,
1076+
reduce(lambda a, b: a - b, [exp.func("COALESCE", field, 0) for field in fields]), # type: ignore
1077+
)
1078+
1079+
1080+
def _null_if_all_null(fields: t.Sequence[exp.Expr], arithmetic: exp.Expr) -> exp.Case:
1081+
"""Returns NULL when every field is NULL, otherwise the result of the arithmetic.
1082+
1083+
The arithmetic is parenthesized because the optimizer replaces the CASE with this branch
1084+
when the condition is statically false (e.g. `1 IS NULL`), and without the parentheses
1085+
the operation would bind to the operators around the macro call.
10741086
"""
10751087
return (
10761088
exp.Case()
10771089
.when(exp.and_(*(field.is_(exp.null()) for field in fields)), exp.null())
1078-
.else_(reduce(lambda a, b: a - b, [exp.func("COALESCE", field, 0) for field in fields])) # type: ignore
1090+
.else_(exp.paren(arithmetic, copy=False))
10791091
)
10801092

10811093

10821094
@macro()
1083-
def safe_div(_: MacroEvaluator, numerator: exp.Expr, denominator: exp.Expr) -> exp.Div:
1095+
def safe_div(_: MacroEvaluator, numerator: exp.Expr, denominator: exp.Expr) -> exp.Paren:
10841096
"""Divides numbers, returns null if the denominator is 0.
10851097
10861098
Example:
10871099
>>> from sqlglot import parse_one
10881100
>>> from sqlmesh.core.macros import MacroEvaluator
10891101
>>> sql = "SELECT @SAFE_DIV(a, b) FROM foo"
10901102
>>> MacroEvaluator().transform(parse_one(sql)).sql()
1091-
'SELECT a / NULLIF(b, 0) FROM foo'
1103+
'SELECT (a / NULLIF(b, 0)) FROM foo'
10921104
"""
1093-
return numerator / exp.func("NULLIF", denominator, 0)
1105+
# The quotient must stay a single operand of whatever operator surrounds the macro call
1106+
return exp.paren(numerator / exp.func("NULLIF", denominator, 0), copy=False)
10941107

10951108

10961109
@macro()

tests/core/test_macros.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from sqlmesh.utils.errors import SQLMeshError
1212
from sqlmesh.utils.metaprogramming import Executable
1313
from sqlmesh.core.macros import RuntimeStage
14+
from sqlmesh.core.model import load_sql_based_model
1415

1516

1617
@pytest.fixture
@@ -1313,3 +1314,51 @@ def render(dialect: str, hash_function: str) -> str:
13131314
render("snowflake", "SHA256")
13141315
== "SELECT SHA256(CONCAT(COALESCE(CAST(a AS VARCHAR), '_sqlmesh_surrogate_key_null_'))) FROM foo"
13151316
)
1317+
1318+
1319+
@pytest.mark.parametrize(
1320+
"projection, expected",
1321+
[
1322+
# GitHub issue #5649: once the optimizer resolves `1 IS NULL`, the CASE collapses into its
1323+
# ELSE branch, and the arithmetic must stay grouped inside the surrounding multiplication.
1324+
(
1325+
"(@SAFE_SUB(price, amount_off)) * (@SAFE_SUB(1, percent_off / 100))",
1326+
'CASE WHEN "s"."amount_off" IS NULL AND "s"."price" IS NULL THEN NULL ELSE (COALESCE("s"."price", 0) - COALESCE("s"."amount_off", 0)) END * (COALESCE(1, 0) - COALESCE("s"."percent_off" / 100, 0))',
1327+
),
1328+
("x * @SAFE_SUB(1, y)", '"s"."x" * (COALESCE(1, 0) - COALESCE("s"."y", 0))'),
1329+
("-@SAFE_SUB(1, y)", '-(COALESCE(1, 0) - COALESCE("s"."y", 0))'),
1330+
("x - @SAFE_ADD(1, y)", '"s"."x" - (COALESCE(1, 0) + COALESCE("s"."y", 0))'),
1331+
("x * @SAFE_ADD(1, y)", '"s"."x" * (COALESCE(1, 0) + COALESCE("s"."y", 0))'),
1332+
(
1333+
"@SAFE_DIV(@SAFE_SUB(1, y), x)",
1334+
'(COALESCE(1, 0) - COALESCE("s"."y", 0)) / NULLIF("s"."x", 0)',
1335+
),
1336+
# The quotient is a single operand of the enclosing operator.
1337+
("x / @SAFE_DIV(price, y)", '"s"."x" / ("s"."price" / NULLIF("s"."y", 0))'),
1338+
("x * @SAFE_DIV(price, y)", '"s"."x" * ("s"."price" / NULLIF("s"."y", 0))'),
1339+
# Standalone usages: the optimizer drops the redundant parentheses around the quotient,
1340+
# while the ELSE branch keeps its grouping.
1341+
("@SAFE_DIV(price, y)", '"s"."price" / NULLIF("s"."y", 0)'),
1342+
(
1343+
"@SAFE_SUB(price, amount_off) + 1",
1344+
'CASE WHEN "s"."amount_off" IS NULL AND "s"."price" IS NULL THEN NULL ELSE (COALESCE("s"."price", 0) - COALESCE("s"."amount_off", 0)) END + 1',
1345+
),
1346+
],
1347+
)
1348+
def test_safe_arithmetic_macros_keep_precedence_after_optimization(
1349+
projection: str, expected: str
1350+
) -> None:
1351+
model = load_sql_based_model(
1352+
d.parse(
1353+
f"""
1354+
MODEL (name db.safe_arithmetic);
1355+
1356+
SELECT {projection} AS result
1357+
FROM (SELECT 1 AS x, 2 AS y, 100 AS price, 25 AS amount_off, 20 AS percent_off) AS s
1358+
"""
1359+
)
1360+
)
1361+
1362+
rendered_projection = model.render_query_or_raise().selects[0]
1363+
1364+
assert rendered_projection.sql() == f'{expected} AS "result"'

0 commit comments

Comments
 (0)