Reject negative ln inputs#22276
Conversation
kumarUjjawal
left a comment
There was a problem hiding this comment.
I think we also need to look into other such log functions which also return NaN.
DataFusion CLI v53.1.0
> > SELECT
ln(2.0) AS ln_pos,
log10(100.0) AS log10_pos,
log(-1.0) AS log_neg,
log2(-1.0) AS log2_neg,
log10(-1.0) AS log10_neg;
+--------------------+-----------+---------+----------+-----------+
| ln_pos | log10_pos | log_neg | log2_neg | log10_neg |
+--------------------+-----------+---------+----------+-----------+
| 0.6931471805599453 | 2.0 | NaN | NaN | NaN |
+--------------------+-----------+---------+----------+-----------+
1 row(s) fetched.
Elapsed 0.004 seconds.
There was a problem hiding this comment.
We don't need a new file for the ln we can extend the already available macro for this.
|
Updated in 48152f0. Covered the other negative-input cases from your example:
Validation run:
One limitation: full |
kumarUjjawal
left a comment
There was a problem hiding this comment.
Can you also look into the power usage of log?
| Float64Type, | ||
| Float64Type, | ||
| _, | ||
| >(&value, &base, checked_log)?, |
There was a problem hiding this comment.
This rewrite can skip the new error. log(-2.0, power(-2.0, 3.0)) should error because the log value is negative, but this can simplify to 3.0. Please only rewrite when the log domain is proven valid.
| } | ||
|
|
||
| /// Returns true if the function is `PowerFunc` | ||
| fn is_pow(func: &ScalarUDF) -> bool { |
There was a problem hiding this comment.
This rewrite can skip the new error. log(-2.0, -2.0) should error, but this can simplify to 1. Please keep the original expression unless the value and base are proven valid
Which issue does this PR close?
ln(-1.0::float8)should error, not return NaN #22271.Rationale for this change
DataFusion currently returns
NaNfor negativelninputs. PostgreSQL raises an error for the same case, and issue #22271 tracks aligningln((-1.0)::float8)with that behavior.This keeps the existing
ln(0)behavior unchanged.What changes are included in this PR?
lnfrom the shared unary math macro to a dedicatedLnFuncimplementation.lnreceives a negativeFloat32orFloat64value.scalar.sltto cover positive column inputs plus negative scalar and column error cases.Are these changes tested?
Yes. I ran:
cargo fmt --check -- datafusion/functions/src/math/mod.rs datafusion/functions/src/math/ln.rscargo check -p datafusion-functionscargo test -p datafusion-functions math::ln --libcargo test -p datafusion-sqllogictest --test sqllogictests -- scalar:586cargo test -p datafusion-sqllogictest --test sqllogictests -- scalar:593cargo test -p datafusion-sqllogictest --test sqllogictests -- scalar:597git diff --checkI also ran
cargo test -p datafusion-sqllogictest --test sqllogictests -- scalar. The changedlnrecords passed, but the full file failed later atscalar.slt:1546+because localaggregate_test_100data was empty in this checkout; those failures were unrelated to this change.Are there any user-facing changes?
Yes.
lnnow errors on negative inputs instead of returningNaN.