fix: ln() of negative or zero should error to match PostgreSQL#22735
Closed
koopatroopa787 wants to merge 1 commit into
Closed
fix: ln() of negative or zero should error to match PostgreSQL#22735koopatroopa787 wants to merge 1 commit into
koopatroopa787 wants to merge 1 commit into
Conversation
PostgreSQL raises an error when ln() is called with a non-positive argument: - ln(0) → "cannot take logarithm of zero" - ln(-x) → "cannot take logarithm of a negative number" DataFusion was returning -Infinity (for zero) and NaN (for negatives). Add validate_ln_input following the same pattern as validate_sqrt_input, and wire it into the make_math_unary_udf! invocation for LnFunc. Update scalar.slt to replace the old -Infinity / NaN expectations with statement error assertions, and add explicit tests for ln(-1.0) and ln(-0.5). Replace the signed_integers column test (which had negative values) with a simple literal positive-value array test. Closes apache#22271
github-actions
Bot
added
sqllogictest
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Aligns ln() behavior with PostgreSQL by raising execution errors for non-positive inputs, and updates SQLLogicTests accordingly.
Changes:
- Added
ln()domain validation to error on0and negative values. - Wired the new validation into the
lnunary UDF. - Updated scalar SQLLogicTests to expect errors and adjusted positive-case coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| datafusion/sqllogictest/test_files/scalar.slt | Updates ln() expectations: non-positive inputs now error; simplifies the positive-value test case. |
| datafusion/functions/src/math/mod.rs | Adds validate_ln_input and attaches it to the ln UDF to enforce PostgreSQL-compatible domain errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+596
to
+602
| # ln(0) errors to match PostgreSQL behavior (issue #22271) | ||
| statement error cannot take logarithm of zero | ||
| select ln(0); | ||
| ---- | ||
| -Infinity | ||
|
|
||
| # ln with columns (round is needed to normalize the outputs of different operating systems) | ||
| # ln of negative numbers errors to match PostgreSQL behavior (issue #22271) | ||
| statement error cannot take logarithm of a negative number | ||
| select ln(-1.0::double); |
Comment on lines
+607
to
+612
| # ln with positive values (array path) | ||
| query RRR rowsort | ||
| select round(ln(a), 5), round(ln(b), 5), round(ln(c), 5) from signed_integers; | ||
| select round(ln(a::double), 5), round(ln(b::double), 5), round(ln(c::double), 5) | ||
| from (values (1, 2, 100)) as t(a, b, c); | ||
| ---- | ||
| 0.69315 NaN 4.81218 | ||
| 1.38629 NULL NULL | ||
| NaN 4.60517 NaN | ||
| NaN 9.21034 NaN | ||
| 0 0.69315 4.60517 |
Contributor
|
@koopatroopa787 We already have a pr for this issue here #22276 |
Contributor
Author
|
Hey @kumarUjjawal, apologies again — didn't see #22276 was already open. Closing this. Will check for existing work before opening PRs going forward, sorry for the noise! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #22271
Rationale for this change
PostgreSQL raises an error when
ln()receives a non-positive argument:ln(0)→ERROR: cannot take logarithm of zeroln(-1)→ERROR: cannot take logarithm of a negative numberDataFusion was silently returning
-Infinityfor zero andNaNfor negatives, diverging from PostgreSQL semantics.What changes are included in this PR?
datafusion/functions/src/math/mod.rs: Addedvalidate_ln_input(same pattern as the existingvalidate_sqrt_input) and passed it as the$VALIDATORargument to theLnFuncmacro invocation.datafusion/sqllogictest/test_files/scalar.slt: Updatedln(0)and negative-value tests to expect errors; added explicitln(-1.0)andln(-0.5)error assertions; replaced thesigned_integerscolumn test (which contained negative values) with a simple positive-literal array test.Are there any user-facing changes?
Yes —
ln(0)andln(<negative>)now raise errors instead of returning-Infinity/NaN. This is a bug fix for PostgreSQL compatibility.🤖 Generated with Claude Code