Skip to content

feat: Allow log with non-integer base on decimals #19372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Yuvraj-cyborg wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: Allow log with non-integer base on decimals #19372

Yuvraj-cyborg wants to merge 7 commits into from
+139 −107

Conversation

@Yuvraj-cyborg
Copy link
Contributor

@Yuvraj-cyborg Yuvraj-cyborg commented Dec 17, 2025 •
edited
Loading

Which issue does this PR close?

Closes #19347

Rationale for this change

Native decimal log() (added in #17023) only supports integer bases. Non-integer bases like log(2.5, x) error out, which is a regression from the previous float-based implementation.

What changes are included in this PR?

Changes :

  • Fallback to f64 computation when base is non-integer
  • Integer bases (2, 10, etc.) still use efficient ilog() algorithm

Refactoring:

  • Unified log_decimal32, log_decimal64, log_decimal128 into single generic log_decimal using num_traits::ToPrimitive.
  • Used ToPrimitive::to_f64() and to_u128()
  • Invalid bases (≤1, NaN, Inf) now return NaN instead of erroring - matches f64::log behavior
    Large Decimal256 values that don't fit in i128 now work via f64 fallback.

Are these changes tested?

Yes:

  • All existing log_decimal* unit tests pass
  • Updated test_log_decimal128_invalid_base - expects NaN instead of error
  • Updated test_log_decimal256_large - now succeeds via fallback

Are there any user-facing changes?

Yes:

-- Previously errored, now works
SELECT log(2.5, 100::decimal(38,0));

-- Invalid base now returns NaN instead of error (consistent with float behavior)
SELECT log(-2, 64::decimal(38,0));  -- Returns NaN

@Yuvraj-cyborg
Copy link
Contributor Author

Yuvraj-cyborg commented Dec 17, 2025

cc: @Jefffrey @martin-g

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Dec 17, 2025
@Yuvraj-cyborg
Copy link
Contributor Author

Yuvraj-cyborg commented Dec 17, 2025

cc: @alamb

Jefffrey
Jefffrey reviewed Dec 20, 2025
View reviewed changes
datafusion/functions/src/math/log.rs Outdated
/// Returns error if base is invalid
fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64, ArrowError> {
if !base.is_finite() || base.trunc() != base {
if !base.is_finite() {
Copy link
Contributor

@Jefffrey Jefffrey Dec 20, 2025

Choose a reason for hiding this comment

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

I think we need to check some of these other error conditions such as base being non-finite or base being less than 1.0, as I don't think they error on float version

Copy link
Contributor Author

@Yuvraj-cyborg Yuvraj-cyborg Dec 20, 2025

Choose a reason for hiding this comment

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

check the recent changes
Using a single trait and unified the previous functions !

cc: @Jefffrey

@Yuvraj-cyborg Yuvraj-cyborg force-pushed the log-with-non-int-base branch 2 times, most recently from f0b96f7 to 160c98e Compare December 20, 2025 15:43
Jefffrey
Jefffrey reviewed Jan 3, 2026
View reviewed changes
datafusion/functions/src/math/log.rs Outdated
Comment on lines 141 to 142
fn unscale_decimal_value<T: ToPrimitive>(value: &T, scale: i8) -> Option<u128> {
let value_u128 = value.to_u128()?;
Copy link
Contributor

@Jefffrey Jefffrey Jan 3, 2026

Choose a reason for hiding this comment

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

I'm not sure about this choice to universally convert to u128, even for decimal32 & decimal64

Copy link
Contributor Author

@Yuvraj-cyborg Yuvraj-cyborg Jan 3, 2026

Choose a reason for hiding this comment

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

Valid ! It's overkill

datafusion/functions/src/math/log.rs

#[test]
fn test_log_decimal128_wrong_base() {
fn test_log_decimal128_invalid_base() {
Copy link
Contributor

@Jefffrey Jefffrey Jan 3, 2026

Choose a reason for hiding this comment

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

Could we move these tests to SLTs

@Yuvraj-cyborg Yuvraj-cyborg force-pushed the log-with-non-int-base branch from f3266bd to cb29663 Compare January 3, 2026 21:03
@Yuvraj-cyborg
Copy link
Contributor Author

Yuvraj-cyborg commented Jan 3, 2026

cc: @Jefffrey

@Jefffrey
Copy link
Contributor

Jefffrey commented Jan 4, 2026

I must admit I am very lost now; it seems this PR has removed all the logic for computing log natively on decimal and now we manually convert to float?

@Yuvraj-cyborg
Copy link
Contributor Author

Yuvraj-cyborg commented Jan 4, 2026

I must admit I am very lost now; it seems this PR has removed all the logic for computing log natively on decimal and now we manually convert to float?

Apologies ! I misunderstood your feedback about u128 and overcorrected by removing the native decimal logic entirely.
I'm fixing it rn

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

Reviewers

@Jefffrey Jefffrey Jefffrey left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow log with non-integer base on decimals

2 participants

@Yuvraj-cyborg @Jefffrey