kosiew commented on code in PR #22564: URL: https://github.com/apache/datafusion/pull/22564#discussion_r3330164811
########## datafusion/functions/src/math/log.rs: ########## Review Comment: I think the same issue applies to the `log(a, a) -> 1` simplification. For example: ```sql select log(column1, column1) from (values (0), (2)) as t(column1); ``` After simplification this returns: ```text 1.0 1.0 ``` but the `a = 0` row would normally evaluate `log(0, 0)` and should raise the new zero-value error. Could we gate this rewrite on proving the value is non-zero, or otherwise preserve the runtime validation path? It would be great to add a regression test that exercises a column value of zero rather than only literal-zero cases. ########## datafusion/functions/src/math/log.rs: ########## Review Comment: Nice improvement on the literal zero handling. I think there is still a gap for non-literal expressions. The guard only catches zero literals before simplification. For expressions like `log(a, power(a, b))`, the rewrite can still eliminate the `log` call entirely. That means rows where `a = 0` never reach `invoke_with_args`, so the new `cannot take logarithm of zero` validation is skipped. For example: ```sql select log(column1, power(column1, 2)) from (values (0), (2)) as t(column1); ``` After the rewrite this returns: ```text 2.0 2.0 ``` but without the rewrite, the first row evaluates `power(0, 2) = 0` and should raise the zero-logarithm error. Could we either avoid this rewrite unless the value can be proven non-zero, or preserve the runtime validation for rows that evaluate to zero? A regression test with a column containing a zero row would help cover this case. -- 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]
