Copilot commented on code in PR #22735: URL: https://github.com/apache/datafusion/pull/22735#discussion_r3349954068
########## datafusion/sqllogictest/test_files/scalar.slt: ########## @@ -593,21 +593,23 @@ select ln(null); ---- NULL -# ln scalar ops with zero edgecases -# please see https://github.com/apache/datafusion/pull/5245#issuecomment-1426828382 -query R rowsort +# 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); Review Comment: These SQLLogicTests assert specific error strings. If DataFusion prefixes/wraps errors (e.g., adding context like function name), these tests can become brittle. Consider using a pattern/regex form (if supported by the harness) or a less-specific match that keys on the stable portion (e.g., just 'cannot take logarithm of zero' / 'cannot take logarithm of a negative number') to reduce churn while still validating the behavior. ########## datafusion/sqllogictest/test_files/scalar.slt: ########## @@ -593,21 +593,23 @@ select ln(null); ---- NULL -# ln scalar ops with zero edgecases -# please see https://github.com/apache/datafusion/pull/5245#issuecomment-1426828382 -query R rowsort +# 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); + +statement error cannot take logarithm of a negative number +select ln(-0.5::double); + +# 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 Review Comment: With the old `signed_integers` query removed, the tests no longer exercise `ln()` over a columnar/vectorized input that includes NULLs and/or domain-invalid values (0/negative) to ensure the new validation is applied consistently in the array path (not just for scalar literals). Consider adding an additional `statement error ...` case that calls `ln(col)` over a VALUES-derived column containing a non-positive value (and optionally NULLs), plus a separate query verifying NULL propagation still works for valid rows. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
