Refactor `log()` signature to use coercion API + fixes #18519

Jefffrey · 2025-11-06T16:38:11Z

Which issue does this PR close?

Rationale for this change

Current log() signature has some drawbacks:

datafusion/datafusion/functions/src/math/log.rs

Lines 78 to 105 in a5eb912

    
           Numeric(1), 
        
           Numeric(2), 
        
           Exact(vec![DataType::Float32, DataType::Float32]), 
        
           Exact(vec![DataType::Float64, DataType::Float64]), 
        
           Exact(vec![ 
        
               DataType::Int64, 
        
               DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0), 
        
           ]), 
        
           Exact(vec![ 
        
               DataType::Float32, 
        
               DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0), 
        
           ]), 
        
           Exact(vec![ 
        
               DataType::Float64, 
        
               DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0), 
        
           ]), 
        
           Exact(vec![ 
        
               DataType::Int64, 
        
               DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0), 
        
           ]), 
        
           Exact(vec![ 
        
               DataType::Float32, 
        
               DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0), 
        
           ]), 
        
           Exact(vec![ 
        
               DataType::Float64, 
        
               DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0), 
        
           ]),

A bit nasty to look at: mixes numeric with exact float/int with exact decimal (of exact precision and scale)
Can't accommodate arbitrary decimals of any precision/scale (this is true for other functions too)

Aim of this PR is to refactor it to use the coercion API, uplifting the API where necessary to make this possible. This simplifies the signature in code, whilst not losing flexibility.

Also other minor refactors are included to log.

What changes are included in this PR?

New TypeSignatureClass variants: Float, Decimal & Numeric

Refactor log() signature to be more in line with it's supported implementations.

Fix issue in log() where ColumnarValue::Scalars were being lost as ColumnarValue::Arrays for the base.

Support null propagation in simplify() for log().

~~Fix issue with calculate_binary_math where it wasn't casting scalars.~~

Are these changes tested?

Added new tests.

Tests for float16, decimal32, decimal64, decimals with different scales/precisions
Test for null propagation (ensure use array input to avoid function inlining)

Are there any user-facing changes?

No.

Jefffrey

~~Note to self: add the tests~~ Done

Jefffrey · 2025-11-06T16:39:49Z

datafusion/sqllogictest/test_files/order.slt

 logical_plan
 01)Sort: log_c11_base_c12 ASC NULLS LAST
-02)--Projection: log(aggregate_test_100.c12, CAST(aggregate_test_100.c11 AS Float64)) AS log_c11_base_c12
+02)--Projection: log(aggregate_test_100.c12, aggregate_test_100.c11) AS log_c11_base_c12


c12 is f64, c11 is f32; now log() will handle casting them to the right types internally for it's implementation

that is probably more efficient than trying to cast externally anyways

Jefffrey · 2025-11-06T16:40:49Z

datafusion/sqllogictest/test_files/decimal.slt

+# TODO: this should be 116.267483321058, error with native decimal log impl
 query R
 select log(2.0, 100000000000000000000000000000000000::decimal(38,0));
 ----
-116.267483321058
+116


This is interesting; we weren't actually making use of the native decimal implementation for log here apparently. The decimal value was actually being casted to float. Fixing the signature to have decimals take precedence (so they don't get casted to float) reveals a bug in our decimal log implementation.

~~Note to self: raise separate issue for this~~

Raised Decimal128 implementation of log loses precision #18524

datafusion/functions/src/utils.rs

Jefffrey · 2025-11-06T16:42:57Z

datafusion/functions/src/math/log.rs

+        // Null propagation
+        if arg_types.iter().any(|dt| dt.is_null()) {
+            return Ok(ExprSimplifyResult::Simplified(lit(
+                ScalarValue::Null.cast_to(&return_type)?
+            )));
+        }


So any log(null, x), log(x, null) or log(null) should be immediately simplified to null.

~~I'll add a test case to show this actually has an effect.~~

Added by 0da79bd

Jefffrey · 2025-11-06T16:44:05Z

datafusion/functions/src/math/log.rs


 impl LogFunc {
    pub fn new() -> Self {
+        // Converts decimals & integers to float64, accepting other floats as is


Main fix here for signature; to me this is more readable than the previous signature, and importantly it accepts any decimals regardless of precision/scale.

~~Note to self: add tests for decimals of different precision/scale if they don't exist~~

Added by 0da79bd

Jefffrey · 2025-11-06T16:45:55Z

datafusion/functions/src/math/log.rs


    // Support overloaded log(base, x) and log(x) which defaults to log(10, x)
    fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
-        let args = ColumnarValue::values_to_arrays(&args.args)?;


This was converting all the args to array, losing optimization opportunity if the base was a scalar. Fix so we maintain the scalar throughout. the value will need to be an array because calculate_binary_math requires its left argument (value for us) to be an array anyway:

datafusion/datafusion/functions/src/utils.rs

Lines 131 to 135 in a5eb912

pub fn calculate_binary_math<L, R, O, F>(

left: &dyn Array,

right: &ColumnarValue,

fun: F,

) -> Result<Arc<PrimitiveArray<O>>>

Jefffrey · 2025-11-06T16:46:48Z

datafusion/functions/src/math/log.rs

-                Float64Type,
-                _,
-            >(value, &base, |x, b| Ok(f64::log(x, b)))?,
-            DataType::Int32 => {


I removed arms for Int32 & Int64 since they should now be casted to Float64 by the new signature; this is functionally equivalent to what the implementations for Int32 & Int64 did anyway

Jefffrey · 2025-11-07T06:24:19Z

datafusion/sqllogictest/test_files/math.slt

+# Null propagation for log
+query TT
+EXPLAIN SELECT log(NULL, c2) from aggregate_simple;
+----
+logical_plan
+01)Projection: Float64(NULL) AS log(NULL,aggregate_simple.c2)
+02)--TableScan: aggregate_simple projection=[]
+physical_plan
+01)ProjectionExec: expr=[NULL as log(NULL,aggregate_simple.c2)]
+02)--DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_simple.csv]]}, file_type=csv, has_header=true


On main this fails like so:

1. query result mismatch: [SQL] EXPLAIN SELECT log(NULL, c2) from aggregate_simple; [Diff] (-expected|+actual) logical_plan - 01)Projection: Float64(NULL) AS log(NULL,aggregate_simple.c2) - 02)--TableScan: aggregate_simple projection=[] + 01)Projection: log(Float64(NULL), aggregate_simple.c2) AS log(NULL,aggregate_simple.c2) + 02)--TableScan: aggregate_simple projection=[c2] physical_plan - 01)ProjectionExec: expr=[NULL as log(NULL,aggregate_simple.c2)] - 02)--DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_simple.csv]]}, file_type=csv, has_header=true + 01)ProjectionExec: expr=[log(NULL, c2@0) as log(NULL,aggregate_simple.c2)] + 02)--RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1 + 03)----DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_simple.csv]]}, projection=[c2], file_type=csv, has_header=true at /Users/jeffrey/Code/datafusion/datafusion/sqllogictest/test_files/math.slt:710

On main it still computes the log; in this PR the log function gets optimized away entirely

datafusion/common/src/scalar/mod.rs

datafusion/expr-common/src/signature.rs

alamb

I went over this PR and it looks really nice to me @Jefffrey - i don't have anything to add -- it is well tested, well commented, and well structured 🏆

Thank you @martin-g for the review 🙏

alamb · 2025-11-07T21:33:43Z

datafusion/sqllogictest/test_files/order.slt

 logical_plan
 01)Sort: log_c11_base_c12 ASC NULLS LAST
-02)--Projection: log(aggregate_test_100.c12, CAST(aggregate_test_100.c11 AS Float64)) AS log_c11_base_c12
+02)--Projection: log(aggregate_test_100.c12, aggregate_test_100.c11) AS log_c11_base_c12


that is probably more efficient than trying to cast externally anyways

## Which issue does this PR close?  Part of apache#14763 and apache#14760 ## Rationale for this change  Current `log()` signature has some drawbacks: https://github.com/apache/datafusion/blob/a5eb9121ccf802dda547897155403b08a4fbf774/datafusion/functions/src/math/log.rs#L78-L105 - A bit nasty to look at: mixes numeric with exact float/int with exact decimal (of exact precision and scale) - Can't accommodate arbitrary decimals of any precision/scale (this is true for other functions too) Aim of this PR is to refactor it to use the coercion API, uplifting the API where necessary to make this possible. This simplifies the signature in code, whilst not losing flexibility. Also other minor refactors are included to log. ## What changes are included in this PR?  New `TypeSignatureClass` variants: Float, Decimal & Numeric Refactor `log()` signature to be more in line with it's supported implementations. Fix issue in `log()` where `ColumnarValue::Scalar`s were being lost as `ColumnarValue::Array`s for the base. Support null propagation in `simplify()` for `log()`. ~~Fix issue with `calculate_binary_math` where it wasn't casting scalars.~~ ## Are these changes tested?  Added new tests. - Tests for float16, decimal32, decimal64, decimals with different scales/precisions - Test for null propagation (ensure use array input to avoid function inlining) ## Are there any user-facing changes?  No.

Refactor log() signature to use coercion API + fixes

dcb3500

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) common Related to common crate functions Changes to functions implementation labels Nov 6, 2025

Jefffrey commented Nov 6, 2025

View reviewed changes

CI checks

2ce83f0

Jefffrey mentioned this pull request Nov 7, 2025

feat: support decimal for math functions: power #18032

Merged

Tests & fix casting dec32/64 to dec128

0da79bd

Jefffrey commented Nov 7, 2025

View reviewed changes

Jefffrey mentioned this pull request Nov 7, 2025

Decimal128 implementation of log loses precision #18524

Open

Add issue links

bfb9e92

Jefffrey marked this pull request as ready for review November 7, 2025 06:57

martin-g reviewed Nov 7, 2025

View reviewed changes

datafusion/common/src/scalar/mod.rs Show resolved Hide resolved

datafusion/expr-common/src/signature.rs Outdated Show resolved Hide resolved

Jefffrey added 2 commits November 7, 2025 22:28

Merge branch 'main' into refactor-log-sig

258158a

review comments

029aa39

alamb approved these changes Nov 7, 2025

View reviewed changes

alamb mentioned this pull request Nov 7, 2025

Andrew Lamb Weekly-ish Open Source plan - 2025-11-03 #18486

Closed

53 tasks

Merge branch 'main' into refactor-log-sig

391f0ca

Jefffrey added this pull request to the merge queue Nov 8, 2025

Merged via the queue into apache:main with commit c826009 Nov 8, 2025
33 checks passed

Jefffrey deleted the refactor-log-sig branch November 8, 2025 05:37

	Numeric(1),
	Numeric(2),
	Exact(vec![DataType::Float32, DataType::Float32]),
	Exact(vec![DataType::Float64, DataType::Float64]),
	Exact(vec![
	DataType::Int64,
	DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
	]),
	Exact(vec![
	DataType::Float32,
	DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
	]),
	Exact(vec![
	DataType::Float64,
	DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
	]),
	Exact(vec![
	DataType::Int64,
	DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
	]),
	Exact(vec![
	DataType::Float32,
	DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
	]),
	Exact(vec![
	DataType::Float64,
	DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
	]),

	pub fn calculate_binary_math<L, R, O, F>(
	left: &dyn Array,
	right: &ColumnarValue,
	fun: F,
	) -> Result<Arc<PrimitiveArray<O>>>

Refactor log() signature to use coercion API + fixes #18519

Refactor log() signature to use coercion API + fixes #18519

Uh oh!

Conversation

Jefffrey commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Jefffrey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jefffrey Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Jefffrey Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Refactor `log()` signature to use coercion API + fixes #18519

Refactor `log()` signature to use coercion API + fixes #18519

Jefffrey commented Nov 6, 2025 •

edited

Loading

Jefffrey left a comment •

edited

Loading

Jefffrey Nov 6, 2025 •

edited

Loading

Jefffrey Nov 6, 2025 •

edited

Loading

Jefffrey Nov 6, 2025 •

edited

Loading