From 44d353ade3a817fd4c79044d2953c8767ae46074 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 21 Oct 2026 15:29:42 -0600 Subject: [PATCH] fix: keep a division's operands grouped on Snowflake and Databricks The guarded division rendered both operands at the lowest precedence and joined them with /, so amount / (ratio * 2) became "amount" / "ratio" * 2. The quotient now wraps each operand at division's precedence, as every other infix does. The zero guards keep the bare operands, since a comparison binds looser than arithmetic. The shared warehouse fixture now pins the quotient of the corpus's negative quotient for both targets. Closes #439 --- pkg-py/src/commons/_definitions/_emit_sql.py | 7 ++++++- pkg-r/NEWS.md | 2 ++ pkg-r/R/definition-emit-sql.R | 16 +++++++++++++--- .../shared/definition-warehouse-sql.json | 12 ++++++++++-- tests/shared/definition-warehouse-sql.json | 12 ++++++++++-- 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/pkg-py/src/commons/_definitions/_emit_sql.py b/pkg-py/src/commons/_definitions/_emit_sql.py index 1e406c347..e58ffc587 100644 --- a/pkg-py/src/commons/_definitions/_emit_sql.py +++ b/pkg-py/src/commons/_definitions/_emit_sql.py @@ -327,8 +327,13 @@ def _divide(node: Ir, state: _State, selected: str | None) -> str: null, so the cases are written out. """ dialect = state.dialect + # The guards compare each operand, which binds looser than arithmetic, so + # only the quotient needs the operands wrapped at division's precedence. numerator = _child(node.attrs["lhs"], 0, "free", state, selected) divisor = _child(node.attrs["rhs"], 0, "free", state, selected) + quotient = _infix( + node.attrs["lhs"], node.attrs["rhs"], "/", _MULTIPLICATIVE, state, selected + ) nan = _non_finite_literal("nan", dialect) infinity = _non_finite_literal("inf", dialect) negative_infinity = _non_finite_literal("-inf", dialect) @@ -339,7 +344,7 @@ def _divide(node: Ir, state: _State, selected: str | None) -> str: f"WHEN {numerator} IS NULL THEN NULL " f"WHEN {is_nan} OR {numerator} = 0 THEN {nan} " f"WHEN {numerator} < 0 THEN {negative_infinity} ELSE {infinity} END " - f"ELSE {numerator} / {divisor} END" + f"ELSE {quotient} END" ) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 079dfcda7..094b8d933 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -1,5 +1,7 @@ # commons (development version) +* A definition that divides by a compound expression, such as `amount / (ratio * 2)`, now compiles to the correct Snowflake and Databricks SQL. The parentheses around the divisor were dropped, so the query divided by `ratio` alone (#439). + * `commons()` gains a `mode` argument. With `mode = "trusted only"`, the agent answers only with trusted calculations. It never writes its own SQL or R, and it tells the user when no trusted calculation answers a question. These answers carry no provenance markers. * `describe_table` reports complete table schemas and documentation without a sampled-row summary. DuckDB descriptions use database-native types. Snowflake and Databricks continue to use native metadata, while other DBI connections infer R column classes from five sampled rows, or zero rows in trusted-only mode (#319). diff --git a/pkg-r/R/definition-emit-sql.R b/pkg-r/R/definition-emit-sql.R index 27885902a..127bf663f 100644 --- a/pkg-r/R/definition-emit-sql.R +++ b/pkg-r/R/definition-emit-sql.R @@ -396,6 +396,17 @@ definition_sql_divide <- function(node, state, dialect, selected) { dialect, selected ) + # The guards compare each operand, which binds looser than arithmetic, so + # only the quotient needs the operands wrapped at division's precedence. + quotient <- definition_sql_infix( + node$lhs, + node$rhs, + "/", + 6L, + state, + dialect, + selected + ) nan <- definition_sql_non_finite_literal("nan", dialect) inf <- definition_sql_non_finite_literal("inf", dialect) neg_inf <- definition_sql_non_finite_literal("-inf", dialect) @@ -407,7 +418,7 @@ definition_sql_divide <- function(node, state, dialect, selected) { "WHEN %s IS NULL THEN NULL ", "WHEN %s OR %s = 0 THEN %s ", "WHEN %s < 0 THEN %s ELSE %s END ", - "ELSE %s / %s END" + "ELSE %s END" ), divisor, numerator, @@ -417,8 +428,7 @@ definition_sql_divide <- function(node, state, dialect, selected) { numerator, neg_inf, inf, - numerator, - divisor + quotient ) } diff --git a/pkg-r/tests/testthat/fixtures/shared/definition-warehouse-sql.json b/pkg-r/tests/testthat/fixtures/shared/definition-warehouse-sql.json index 07781eb60..a5489d876 100644 --- a/pkg-r/tests/testthat/fixtures/shared/definition-warehouse-sql.json +++ b/pkg-r/tests/testthat/fixtures/shared/definition-warehouse-sql.json @@ -74,8 +74,16 @@ "SQL(snowflake)": { "notes_contain": ["newline characters"] } }, "survey::negative quotient": { - "SQL(snowflake)": { "code_contains": ["CASE WHEN"] }, - "SQL(databricks)": { "notes_contain": ["negative zero divisor"] } + "SQL(snowflake)": { + "code_contains": [ + "CASE WHEN \"ratio\" + 1.0 = 0 THEN", + "ELSE -(\"amount\" - 2) / (\"ratio\" + 1.0) END" + ] + }, + "SQL(databricks)": { + "code_contains": ["ELSE -(`amount` - 2) / (`ratio` + 1.0) END"], + "notes_contain": ["negative zero divisor"] + } } } }, diff --git a/tests/shared/definition-warehouse-sql.json b/tests/shared/definition-warehouse-sql.json index 07781eb60..a5489d876 100644 --- a/tests/shared/definition-warehouse-sql.json +++ b/tests/shared/definition-warehouse-sql.json @@ -74,8 +74,16 @@ "SQL(snowflake)": { "notes_contain": ["newline characters"] } }, "survey::negative quotient": { - "SQL(snowflake)": { "code_contains": ["CASE WHEN"] }, - "SQL(databricks)": { "notes_contain": ["negative zero divisor"] } + "SQL(snowflake)": { + "code_contains": [ + "CASE WHEN \"ratio\" + 1.0 = 0 THEN", + "ELSE -(\"amount\" - 2) / (\"ratio\" + 1.0) END" + ] + }, + "SQL(databricks)": { + "code_contains": ["ELSE -(`amount` - 2) / (`ratio` + 1.0) END"], + "notes_contain": ["negative zero divisor"] + } } } },