From 312eec68e67c64a4dcdb072d8ad492b404779a8a Mon Sep 17 00:00:00 2001 From: Yoongbok Lee <7127607+U0001F3A2@users.noreply.github.com> Date: Sat, 18 Jul 2026 01:07:37 +0900 Subject: [PATCH 1/2] fix: preserve precision in log() with mixed-width float arguments `log(base, value)` resolved its result type and computed only from the value (the last argument). When the base was a wider float than the value, `calculate_binary_math` narrowed the base to the value's type, so `log(Float64, Float32)` computed in f32 and lost precision: log(16777217::float8, 2.0::float4) => 0.041666668 (Float32) log(16777217::float8, 2.0::float8) => 0.041666666517376175 (Float64) Resolve the result type to the widest float among all arguments and compute there, widening the value to match instead of narrowing the base. The `log(base, 1)` / `log(a, a)` simplifications now emit their literal with that resolved type too, so a wider value no longer leaves the optimizer with a mismatched plan schema. Integer/decimal arguments already coerce to Float64; null arguments do not constrain the width. Homogeneous float widths and the decimal-value paths are unchanged. Existing math.slt and scalar.slt cases that asserted the narrowed type for `log(Float64 base, narrower value)` are updated, since they encoded the bug. Closes #22581 Co-Authored-By: Claude --- datafusion/functions/src/math/log.rs | 163 +++++++++++++++--- datafusion/sqllogictest/test_files/math.slt | 44 ++++- datafusion/sqllogictest/test_files/scalar.slt | 4 +- 3 files changed, 179 insertions(+), 32 deletions(-) diff --git a/datafusion/functions/src/math/log.rs b/datafusion/functions/src/math/log.rs index 2ca2ed1b572be..9e6263c63f1ac 100644 --- a/datafusion/functions/src/math/log.rs +++ b/datafusion/functions/src/math/log.rs @@ -21,6 +21,7 @@ use super::power::PowerFunc; use crate::utils::calculate_binary_math; use arrow::array::{Array, ArrayRef}; +use arrow::compute::cast; use arrow::datatypes::{ DataType, Decimal32Type, Decimal64Type, Decimal128Type, Decimal256Type, Float16Type, Float32Type, Float64Type, @@ -100,6 +101,30 @@ impl LogFunc { } } +/// The float type `log` computes and returns for the given argument types: the +/// widest float present, so a wider base is never narrowed to a narrower value +/// (issue #22581). Integer and decimal arguments are coerced to `Float64` by the +/// signature, and null arguments do not constrain the width. When no concrete +/// float is present the result is `Float64`. +fn log_float_type(arg_types: &[DataType]) -> DataType { + let mut rank = 0u8; + for arg_type in arg_types { + let arg_rank = match arg_type { + DataType::Float16 => 1, + DataType::Float32 => 2, + DataType::Null => continue, + // Float64 and any coerced integer/decimal argument. + _ => 3, + }; + rank = rank.max(arg_rank); + } + match rank { + 1 => DataType::Float16, + 2 => DataType::Float32, + _ => DataType::Float64, + } +} + /// Checks if the base is valid for the efficient integer logarithm algorithm. #[inline] fn is_valid_integer_base(base: f64) -> bool { @@ -195,12 +220,12 @@ impl ScalarUDFImpl for LogFunc { } fn return_type(&self, arg_types: &[DataType]) -> Result { - // Check last argument (value) - match &arg_types.last().ok_or(plan_datafusion_err!("No args"))? { - DataType::Float16 => Ok(DataType::Float16), - DataType::Float32 => Ok(DataType::Float32), - _ => Ok(DataType::Float64), + if arg_types.is_empty() { + return Err(plan_datafusion_err!("No args")); } + // Compute in the widest float of all arguments so a wider base is not + // narrowed to the value's type (issue #22581). + Ok(log_float_type(arg_types)) } fn output_ordering(&self, input: &[ExprProperties]) -> Result { @@ -247,26 +272,39 @@ impl ScalarUDFImpl for LogFunc { let value = value.to_array(args.number_rows)?; let output: ArrayRef = match value.data_type() { - DataType::Float16 => { - calculate_binary_math::( - &value, - &base, - |value, base| Ok(value.log(base)), - )? - } - DataType::Float32 => { - calculate_binary_math::( - &value, - &base, - |value, base| Ok(value.log(base)), - )? - } - DataType::Float64 => { - calculate_binary_math::( - &value, - &base, - |value, base| Ok(value.log(base)), - )? + DataType::Float16 | DataType::Float32 | DataType::Float64 => { + // Compute in the widest float of base and value so a wider base + // is not narrowed to the value's type (issue #22581). + let target = + log_float_type(&[base.data_type(), value.data_type().clone()]); + let value = if value.data_type() == &target { + value + } else { + cast(&value, &target)? + }; + match target { + DataType::Float16 => { + calculate_binary_math::( + &value, + &base, + |value, base| Ok(value.log(base)), + )? + } + DataType::Float32 => { + calculate_binary_math::( + &value, + &base, + |value, base| Ok(value.log(base)), + )? + } + _ => { + calculate_binary_math::( + &value, + &base, + |value, base| Ok(value.log(base)), + )? + } + } } DataType::Decimal32(_, scale) => { calculate_binary_math::( @@ -363,8 +401,11 @@ impl ScalarUDFImpl for LogFunc { Expr::Literal(value, _) if value == ScalarValue::new_one(&number_datatype)? => { + // The zero must carry the resolved return type, not the base's, + // so a wider value does not leave the plan schema inconsistent + // (issue #22581). Ok(ExprSimplifyResult::Simplified(lit(ScalarValue::new_zero( - &info.get_data_type(&base)?, + &return_type, )?))) } Expr::ScalarFunction(ScalarFunction { func, mut args }) @@ -376,7 +417,7 @@ impl ScalarUDFImpl for LogFunc { number => { if number == base { Ok(ExprSimplifyResult::Simplified(lit(ScalarValue::new_one( - &number_datatype, + &return_type, )?))) } else { let args = match num_args { @@ -1205,4 +1246,72 @@ mod tests { } } } + + #[test] + fn test_log_return_type_uses_widest_float() { + use DataType::*; + let f = LogFunc::new(); + + // A wider base must not be narrowed to the value's type (#22581). + assert_eq!(f.return_type(&[Float64, Float32]).unwrap(), Float64); + assert_eq!(f.return_type(&[Float32, Float64]).unwrap(), Float64); + assert_eq!(f.return_type(&[Float32, Float16]).unwrap(), Float32); + assert_eq!(f.return_type(&[Float16, Float32]).unwrap(), Float32); + // Homogeneous inputs keep their type. + assert_eq!(f.return_type(&[Float16, Float16]).unwrap(), Float16); + assert_eq!(f.return_type(&[Float32, Float32]).unwrap(), Float32); + assert_eq!(f.return_type(&[Float64, Float64]).unwrap(), Float64); + assert_eq!(f.return_type(&[Float32]).unwrap(), Float32); + // Null does not constrain the width; all-null falls back to Float64. + assert_eq!(f.return_type(&[Float32, Null]).unwrap(), Float32); + assert_eq!(f.return_type(&[Null, Float32]).unwrap(), Float32); + assert_eq!(f.return_type(&[Null, Null]).unwrap(), Float64); + } + + #[test] + fn test_log_mixed_width_preserves_base_precision() { + // Issue #22581: log(Float64 base, Float32 value) narrowed the base to + // Float32, losing precision. 16777217 is not representable in f32. + let arg_fields = vec![ + Field::new("a", DataType::Float64, false).into(), + Field::new("a", DataType::Float32, false).into(), + ]; + let args = ScalarFunctionArgs { + args: vec![ + ColumnarValue::Scalar(ScalarValue::Float64(Some(16777217.0))), // base + ColumnarValue::Scalar(ScalarValue::Float32(Some(2.0))), // value + ], + arg_fields, + number_rows: 1, + return_field: Field::new("f", DataType::Float64, true).into(), + config_options: Arc::new(ConfigOptions::default()), + }; + let result = LogFunc::new() + .invoke_with_args(args) + .expect("failed to initialize function log"); + + match result { + ColumnarValue::Array(arr) => { + let floats = as_float64_array(&arr) + .expect("result should be a Float64Array, not narrowed to Float32"); + assert_eq!(floats.len(), 1); + + let expected = 2.0_f64.log(16777217.0); + assert!( + (floats.value(0) - expected).abs() < 1e-15, + "got {}, expected f64-precision {expected}", + floats.value(0) + ); + // The lossy f32 computation differs in the 8th decimal; the + // result must not match it. + let lossy = 2.0_f32.log(16777217.0_f32) as f64; + assert!( + (floats.value(0) - lossy).abs() > 1e-9, + "result {} still matches the lossy f32 value {lossy}", + floats.value(0) + ); + } + ColumnarValue::Scalar(_) => panic!("Expected an array value"), + } + } } diff --git a/datafusion/sqllogictest/test_files/math.slt b/datafusion/sqllogictest/test_files/math.slt index 583d6f6777865..9488955132a22 100644 --- a/datafusion/sqllogictest/test_files/math.slt +++ b/datafusion/sqllogictest/test_files/math.slt @@ -920,22 +920,60 @@ logical_plan 02)--TableScan: aggregate_simple projection=[] physical_plan DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_simple.csv]]}, projection=[NULL as log(NULL,aggregate_simple.c2)], file_type=csv, has_header=true -# Float 16/32/64 for log +# log result type is the widest float of base and value, so a wider base is +# not narrowed to the value's type (#22581). Here base 2.5 is Float64. query RT SELECT log(2.5, arrow_cast(10.9, 'Float16')), arrow_typeof(log(2.5, arrow_cast(10.9, 'Float16'))); ---- -2.6074219 Float16 +2.606835742462 Float64 query RT SELECT log(2.5, 10.9::float), arrow_typeof(log(2.5, 10.9::float)); ---- -2.606992 Float32 +2.606992159958 Float64 query RT SELECT log(2.5, 10.9::double), arrow_typeof(log(2.5, 10.9::double)); ---- 2.606992198152 Float64 +# Homogeneous float widths are preserved +query RT +SELECT log(arrow_cast(2.5, 'Float16'), arrow_cast(10.9, 'Float16')), arrow_typeof(log(arrow_cast(2.5, 'Float16'), arrow_cast(10.9, 'Float16'))); +---- +2.6074219 Float16 + +query RT +SELECT log(2.5::float, 10.9::float), arrow_typeof(log(2.5::float, 10.9::float)); +---- +2.606992 Float32 + +# #22581: a Float64 base must keep full precision against a Float32 value. +# 16777217 is not representable in Float32, so an f32 computation would round it. +query RT +SELECT log(arrow_cast(16777217, 'Float64'), arrow_cast(2.0, 'Float32')), arrow_typeof(log(arrow_cast(16777217, 'Float64'), arrow_cast(2.0, 'Float32'))); +---- +0.041666666517 Float64 + +# A NULL value takes the resolved (widest concrete) float type +query RT +SELECT log(2.5::float, NULL), arrow_typeof(log(2.5::float, NULL)); +---- +NULL Float32 + +# #22581: the log(base, 1) simplification must carry the resolved (widest) +# type, or a Float16 base with a wider literal 1 fails the optimizer's +# schema-compatibility check at planning time. +query RT +SELECT log(arrow_cast(2.5, 'Float16'), 1.0::double), arrow_typeof(log(arrow_cast(2.5, 'Float16'), 1.0::double)); +---- +0 Float64 + +query RT +SELECT log(arrow_cast(2.5, 'Float16'), arrow_cast(1.0, 'Float16')), arrow_typeof(log(arrow_cast(2.5, 'Float16'), arrow_cast(1.0, 'Float16'))); +---- +0 Float16 + # lcm with array and scalar query I diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 7666b680e16a8..42367f03f0a7e 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -694,8 +694,8 @@ select log(2,arrow_cast(a, 'Float64')), log(4,arrow_cast(b, 'Float32')) from sig ---- 1 NaN 2 NULL -NaN 3.321928 -NaN 6.643856 +NaN 3.321928094887 +NaN 6.643856189775 ## log10 From d58ac219fa1ef1cff4b0a65c3a7aaca309b75c34 Mon Sep 17 00:00:00 2001 From: Yoongbok Lee <7127607+U0001F3A2@users.noreply.github.com> Date: Sat, 18 Jul 2026 11:05:31 +0900 Subject: [PATCH 2/2] test: update test_simplify_log expectations to log's Float64 return type The log() simplifications `log(x, 1) ===> 0` and `log(x, x) ===> 1` now emit a constant carrying log's resolved Float64 return type rather than the base column's Int64 type (issue #22581), keeping the simplified literal consistent with the function's output schema. Update the corresponding golden expectations in the core integration test that were still asserting Int64. Co-Authored-By: Claude --- datafusion/core/tests/expr_api/simplification.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/datafusion/core/tests/expr_api/simplification.rs b/datafusion/core/tests/expr_api/simplification.rs index e9a975239a481..046655d5ed636 100644 --- a/datafusion/core/tests/expr_api/simplification.rs +++ b/datafusion/core/tests/expr_api/simplification.rs @@ -607,14 +607,17 @@ fn test_simplify_with_cycle_count( #[test] fn test_simplify_log() { // Log(c3, 1) ===> 0 + // The zero carries log's Float64 return type, not the base's Int64, so the + // simplified literal stays consistent with the plan schema (issue #22581). { let expr = log(col("c3_non_null"), lit(1)); - test_simplify(expr, lit(0i64)); + test_simplify(expr, lit(0.0f64)); } // Log(c3, c3) ===> 1 + // The one carries log's Float64 return type, not the base's Int64 (#22581). { let expr = log(col("c3_non_null"), col("c3_non_null")); - let expected = lit(1i64); + let expected = lit(1.0f64); test_simplify(expr, expected); } // Log(c3, Power(c3, c4)) ===> c4