Skip to content

Conversation

@shifluxxc
Copy link
Contributor

@shifluxxc shifluxxc commented Dec 14, 2025

Which issue does this PR close?

Rationale for this change

Previously, the log function would fail when operating on decimal values with negative scales.

Negative scales in decimals represent values where the scale indicates padding zeros to the right (e.g., Decimal128(38, -2) with value 100 represents 10000). This PR restores support for negative-scale decimals in the log function by implementing the logarithmic property: log_base(value * 10^(-scale)) = log_base(value) + (-scale) * log_base(10).

What changes are included in this PR?

  1. Enhanced log_decimal128 function:

    • Added support for negative scales using the logarithmic property
    • For negative scales, computes log_base(value) + (-scale) * log_base(10) instead of trying to convert to unscaled value
    • Added detection for negative-scale decimals in both the number and base arguments
    • Skips simplification when negative scales are detected to avoid errors with ScalarValue (which doesn't support negative scales yet)
  2. Added comprehensive tests:

    • Unit tests in log.rs for negative-scale decimals with various bases (2, 3, 10)
    • SQL logic tests in decimal.slt using scientific notation (e.g., 1e4, 8e1) to create decimals with negative scales

Are these changes tested?

Yes, this PR includes comprehensive tests:

  1. Unit tests:

    • test_log_decimal128_negative_scale: Tests array inputs with negative scales
    • test_log_decimal128_negative_scale_base2: Tests with base 2 and negative scales
    • test_log_decimal128_negative_scale_scalar: Tests scalar inputs with negative scales
  2. SQL logic tests:

    • Tests for unary log with negative scales (log(1e4))
    • Tests for binary log with explicit base 10 (log(10, 1e4))
    • Tests for binary log with base 2 (log(2.0, 8e1), log(2.0, 16e1))
    • Tests for different negative scale values (log(5e3))
    • Tests for array operations with negative scales
    • Tests for different bases (2, 3, 10) with negative-scale decimals

All tests pass successfully.

Are there any user-facing changes?

Yes, this is a user-facing change:

Before: The log function would fail with an error when operating on decimal values with negative scales:

-- This would fail
SELECT log(1e4);  -- Error: Negative scale is not supported

After: The log function now correctly handles decimal values with negative scales:

-- This now works
SELECT log(1e4);  -- Returns 4.0 (log10(10000))
SELECT log(2.0, 8e1);  -- Returns ~6.32 (log2(80))

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Dec 14, 2025
Comment on lines 125 to 134
let value_f64 = value as f64;

// Compute log_base(value) - use natural log and convert
// log_base(x) = ln(x) / ln(base)
let log_value = value_f64.ln() / base.ln();

// Add the adjustment: (-scale) * log_base(10)
// log_base(10) = ln(10) / ln(base)
let log_10_base = 10.0_f64.ln() / base.ln();
let adjustment = (-scale as f64) * log_10_base;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if we're better off just casting to float and doing log there? There seem to be multiple log operations here, does it provide more accuracy?

@Jefffrey Jefffrey changed the title Fix regression for negative-scale decimals in log Fix regression for negative-scale decimal128 in log Dec 19, 2025
@Jefffrey Jefffrey added this pull request to the merge queue Dec 19, 2025
Merged via the queue into apache:main with commit cbf33d1 Dec 19, 2025
12 checks passed
@Jefffrey
Copy link
Contributor

Thanks @shifluxxc & @martin-g

@xudong963
Copy link
Member

Thanks for the fix @shifluxxc , do you mind cherry-picking the PR to branch-52, then we could include it in v52.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants