kosiew commented on code in PR #22564:
URL: https://github.com/apache/datafusion/pull/22564#discussion_r3324384347
##########
datafusion/functions/src/math/log.rs:
##########
@@ -247,27 +269,33 @@ impl ScalarUDFImpl for LogFunc {
let value = value.to_array(args.number_rows)?;
let output: ArrayRef = match value.data_type() {
- DataType::Float16 => {
- calculate_binary_math::<Float16Type, Float16Type, Float16Type,
_>(
- &value,
- &base,
- |value, base| Ok(value.log(base)),
- )?
- }
- DataType::Float32 => {
- calculate_binary_math::<Float32Type, Float32Type, Float32Type,
_>(
- &value,
- &base,
- |value, base| Ok(value.log(base)),
- )?
- }
- DataType::Float64 => {
- calculate_binary_math::<Float64Type, Float64Type, Float64Type,
_>(
- &value,
- &base,
- |value, base| Ok(value.log(base)),
- )?
- }
+ DataType::Float16 => calculate_binary_math::<
Review Comment:
Small cleanup suggestion: the three float branches now repeat the same
validation and log pattern.
It might be worth using a small helper for the callback body, something like
`validate_log_value(value as f64)?; Ok(value.log(base))`, if that can be made
generic over the float types cleanly. If not, the current duplication seems
fine to me.
##########
datafusion/functions/src/math/log.rs:
##########
Review Comment:
In addition, I think the new zero-value invariant is not being enforced
end-to-end yet because expression simplification can bypass `invoke_with_args`.
For example, `SELECT log(0, 0)` still returns `1.0` through the existing
`log(a, a) => 1` rewrite, even though the value argument is zero and should now
fail with `cannot take logarithm of zero`.
I verified this after rebuilding `datafusion-cli`: `select log(0,0);`
returns `1.0`, while `select log(0);` and `select log(2,0);` both error as
expected.
Could you please either make the simplification rules reject zero literals
for the value argument, or enforce this invariant before or inside
simplification and constant folding? It would also be good to add regression
coverage for the binary optimized path.
--
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]
