Skip to content

fix(duckdb): division by zero returns inf, not NULL - #7969

Merged
georgesittas merged 1 commit into
tobymao:mainfrom
Endika:duckdb-safe-division
Jul 28, 2026
Merged

fix(duckdb): division by zero returns inf, not NULL#7969
georgesittas merged 1 commit into
tobymao:mainfrom
Endika:duckdb-safe-division

Conversation

@Endika

@Endika Endika commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

DuckDB is declared SAFE_DIVISION = True, which the docstring defines as "whether division by zero throws an error (False) or returns NULL (True)". Since DuckDB 1.1.0 neither is true: float division follows IEEE 754, so 1 / 0 is inf, -1 / 0 is -inf and 0 / 0 is nan. NULL only comes back with ieee_floating_point_ops = false, which is not the default. Checked against DuckDB 1.5.5:

SELECT 1/0    -> inf
SELECT 0/0    -> nan
SELECT -1/0   -> -inf
SET ieee_floating_point_ops = false;
SELECT 1/0    -> NULL

The effect is in div_sql, which wraps the divisor in NULLIF(b, 0) when the source dialect is NULL-safe and the target is not. DuckDB was declared safe, so the wrap was skipped and a / b read as MySQL stayed a / b in DuckDB, which evaluates to inf where MySQL gives NULL. In the other direction sqlglot emulated a NULL that DuckDB never produces.

Dropping the flag changes exactly one entry of the existing test_safe_div matrix, the DuckDB target. Generating from DuckDB no longer wraps the divisor, since there is no NULL to emulate.

I realise that matrix entry was written deliberately, and that neither value of a two-state flag really describes inf. If you would rather model the third state explicitly, or keep the current behaviour for compatibility, say so and I will close this.

DuckDB has followed IEEE 754 since 1.1.0, so the NULLIF that the
generator adds for NULL-safe sources was wrongly skipped.
@georgesittas

Copy link
Copy Markdown
Collaborator

Since this concerns transpilation, I think we need to distinguish duckdb's inf semantics from what the existing two modes represent, because we can still produce unfaithful SQL under this PR. We should also handle the null-safe semantics when it's specified as a dialect instance option (dialect="duckdb, ieee_floating_point_ops = false").

Is this something you'd be willing to work on?

@georgesittas georgesittas self-assigned this Jul 27, 2026
@Endika

Endika commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Since this concerns transpilation, I think we need to distinguish duckdb's inf semantics from what the existing two modes represent, because we can still produce unfaithful SQL under this PR. We should also handle the null-safe semantics when it's specified as a dialect instance option (dialect="duckdb, ieee_floating_point_ops = false").

Is this something you'd be willing to work on?

Here is the shape I have in mind — tell me if it matches yours before I write it.

Three-state semantics. Replace the boolean with a DivisionByZero enum on Dialect: ERROR (the default), NULL (SQLite, MySQL, Hive, ClickHouse) and IEEE (DuckDB — inf / -inf / nan). SAFE_DIVISION is only read in two places, parser.py where the Div is tagged and div_sql where the wrap is decided, so the change stays contained.

div_sql would then emulate per source → target pair instead of per boolean:

source → target emitted
NULLERROR / IEEE a / NULLIF(b, 0) — today's behaviour, plus the DuckDB target this PR is about
IEEENULL / ERROR nothing today; this is the unfaithful case you mean
ERROR → anything nothing, a raise is not reproducible

For IEEE → NULL/ERROR I can emit a CASE that yields the target's own infinity where it has one (Postgres 'Infinity'::float8) and leave it untouched where it does not (MySQL). Or keep that out of this PR. Your call.

Instance setting. ieee_floating_point_ops added to DuckDB's SUPPORTED_SETTINGS, following variant_extract_is_json_extract in Presto, plus an instance-level accessor so that Dialect.get_or_raise("duckdb, ieee_floating_point_ops = false") resolves to NULL rather than IEEE.

One decision I would rather leave to you. SAFE_DIVISION is a public class attribute that third-party dialects set. I would keep it settable and honour it as an alias for NULL, so nothing downstream breaks silently, and let the enum take precedence when both are given. The alternative is a derived read-only property, which is cleaner but turns existing assignments into silent no-ops. Which would you prefer?

@georgesittas georgesittas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just get this in for now, I don't think it's worth dealing with the general case, because it will add overhead to division which is a pretty common operation. Getting rid of the nullif when targeting unsafe dialects is an improvement on its own because it results in runtime failures vs silently incorrect null values.

@georgesittas
georgesittas merged commit 3e1962f into tobymao:main Jul 28, 2026
8 checks passed
georgesittas pushed a commit that referenced this pull request Jul 28, 2026
DuckDB has followed IEEE 754 since 1.1.0, so the NULLIF that the
generator adds for NULL-safe sources was wrongly skipped.
@Endika
Endika deleted the duckdb-safe-division branch July 28, 2026 11:24
PiyaDaswadkar pushed a commit to PiyaDaswadkar/sqlglot that referenced this pull request Jul 29, 2026
DuckDB has followed IEEE 754 since 1.1.0, so the NULLIF that the
generator adds for NULL-safe sources was wrongly skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants