kosiew commented on code in PR #22564: URL: https://github.com/apache/datafusion/pull/22564#discussion_r3328063647
########## datafusion/functions/src/math/log.rs: ########## Review Comment: Thanks for adding the zero-literal guard. I think there's still a gap here. The new check is added below at lines 405-417, but this `log(a, power(a, b)) => b` rewrite runs first. That means an expression like `log(0, power(0, 2))` can be simplified directly to `2` without ever calling `invoke_with_args`, even though the value argument is zero and should produce `cannot take logarithm of zero`. Could we make sure the zero-value validation runs before rewrites that can eliminate the log expression, or alternatively have this rewrite reject cases where the base or value can be proven to be zero? Otherwise the zero-value invariant can still be bypassed through simplification. -- 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]
