You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Weijun-H (via GitHub)" <gi...@apache.org> on 2025/10/04 15:02:37 UTC
[PR] feat: Implement log for Decimal256 [datafusion]
Weijun-H opened a new pull request, #17918:
URL: https://github.com/apache/datafusion/pull/17918
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #17555
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#issuecomment-3631153456
Hi @Weijun-H , do you need a hand here? I can continue research and do follow-ups.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#discussion_r2404020205
##########
datafusion/functions/src/math/log.rs:
##########
@@ -134,15 +134,60 @@ fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64, ArrowError>
}
}
-/// Binary function to calculate an integer logarithm of Decimal128 `value` using `base` base
-/// Returns error if base is invalid or if value is out of bounds of Decimal128
+/// Binary function to calculate an integer logarithm of Decimal256 `value` using `base` base
+/// Returns error if base is invalid
Review Comment:
The comment says 'integer logarithm' but the function returns f64, not an integer. This should be corrected to 'logarithm' or clarify that it returns the floor of the logarithm.
##########
datafusion/functions/src/math/log.rs:
##########
@@ -134,15 +134,60 @@ fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64, ArrowError>
}
}
-/// Binary function to calculate an integer logarithm of Decimal128 `value` using `base` base
-/// Returns error if base is invalid or if value is out of bounds of Decimal128
+/// Binary function to calculate an integer logarithm of Decimal256 `value` using `base` base
+/// Returns error if base is invalid
fn log_decimal256(value: i256, scale: i8, base: f64) -> Result<f64, ArrowError> {
- match value.to_i128() {
- Some(value) => log_decimal128(value, scale, base),
- None => Err(ArrowError::NotYetImplemented(format!(
- "Log of Decimal256 larger than Decimal128 is not yet supported: {value}"
- ))),
+ if !base.is_finite() || base.trunc() != base {
+ return Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer base: {base}"
+ )));
+ }
+ if (base as u32) < 2 {
+ return Err(ArrowError::ComputeError(format!(
+ "Log base must be greater than 1: {base}"
+ )));
+ }
+
+ // Try to convert to i128 for faster calculation if possible
+ if let Some(value_i128) = value.to_i128() {
+ let unscaled_value = decimal128_to_i128(value_i128, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ return Ok(log_value as f64);
+ } else {
+ return Ok(f64::NAN);
+ }
+ }
+
+ // For values that don't fit in i128, use f64 approximation
+ let unscaled_value = decimal256_to_i256(value, scale)?;
+
+ // Check if the value is non-positive
+ if !unscaled_value.is_positive() {
+ return Ok(f64::NAN);
+ }
+
+ // Convert i256 to f64 for logarithm calculation
+ // Note: This may lose precision for very large numbers, but that's acceptable
+ // for logarithm calculation since the result is relatively small
+ let value_f64 = i256_to_f64(unscaled_value);
+ let log_value = value_f64.log(base);
+
+ Ok(log_value.floor())
+}
+
+/// Converts i256 to f64 for logarithm calculation
+/// This may lose precision for very large numbers but is acceptable for log calculation
+fn i256_to_f64(value: i256) -> f64 {
+ // Try to convert directly if it fits in i128
+ if let Some(value_i128) = value.to_i128() {
+ return value_i128 as f64;
}
+
+ // For larger values, use string conversion (less efficient but more accurate)
+ // Parse the string representation
+ let value_str = value.to_string();
+ value_str.parse::<f64>().unwrap_or(f64::INFINITY)
Review Comment:
String conversion and parsing is inefficient for i256 to f64 conversion. Consider implementing a more direct conversion method using bit manipulation or splitting the i256 into manageable chunks.
```suggestion
// For larger values, convert using bit manipulation
// i256 is represented as 4 u64 limbs (little-endian)
let limbs = value.0;
let mut abs = [0u64; 4];
let negative = value.is_negative();
if negative {
// Two's complement: invert and add 1
let mut carry = true;
for (i, limb) in limbs.iter().enumerate() {
let inv = !limb;
let (res, overflow) = if carry {
inv.overflowing_add(1)
} else {
(inv, false)
};
abs[i] = res;
carry = overflow;
}
} else {
abs.copy_from_slice(&limbs);
}
// Combine limbs into f64
let mut result = 0f64;
for i in (0..4).rev() {
result = result * (1u64 << 64) as f64 + abs[i] as f64;
}
if negative {
-result
} else {
result
}
```
##########
datafusion/functions/src/math/log.rs:
##########
@@ -134,15 +134,60 @@ fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64, ArrowError>
}
}
-/// Binary function to calculate an integer logarithm of Decimal128 `value` using `base` base
-/// Returns error if base is invalid or if value is out of bounds of Decimal128
+/// Binary function to calculate an integer logarithm of Decimal256 `value` using `base` base
+/// Returns error if base is invalid
fn log_decimal256(value: i256, scale: i8, base: f64) -> Result<f64, ArrowError> {
- match value.to_i128() {
- Some(value) => log_decimal128(value, scale, base),
- None => Err(ArrowError::NotYetImplemented(format!(
- "Log of Decimal256 larger than Decimal128 is not yet supported: {value}"
- ))),
+ if !base.is_finite() || base.trunc() != base {
+ return Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer base: {base}"
+ )));
+ }
+ if (base as u32) < 2 {
+ return Err(ArrowError::ComputeError(format!(
+ "Log base must be greater than 1: {base}"
+ )));
+ }
+
+ // Try to convert to i128 for faster calculation if possible
+ if let Some(value_i128) = value.to_i128() {
+ let unscaled_value = decimal128_to_i128(value_i128, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ return Ok(log_value as f64);
+ } else {
+ return Ok(f64::NAN);
+ }
+ }
+
+ // For values that don't fit in i128, use f64 approximation
+ let unscaled_value = decimal256_to_i256(value, scale)?;
+
+ // Check if the value is non-positive
+ if !unscaled_value.is_positive() {
+ return Ok(f64::NAN);
+ }
+
+ // Convert i256 to f64 for logarithm calculation
+ // Note: This may lose precision for very large numbers, but that's acceptable
+ // for logarithm calculation since the result is relatively small
+ let value_f64 = i256_to_f64(unscaled_value);
+ let log_value = value_f64.log(base);
+
+ Ok(log_value.floor())
+}
+
+/// Converts i256 to f64 for logarithm calculation
+/// This may lose precision for very large numbers but is acceptable for log calculation
+fn i256_to_f64(value: i256) -> f64 {
+ // Try to convert directly if it fits in i128
+ if let Some(value_i128) = value.to_i128() {
+ return value_i128 as f64;
}
+
+ // For larger values, use string conversion (less efficient but more accurate)
+ // Parse the string representation
+ let value_str = value.to_string();
+ value_str.parse::<f64>().unwrap_or(f64::INFINITY)
Review Comment:
Using `f64::INFINITY` as a fallback for parse errors could mask legitimate parsing failures and produce incorrect logarithm results. Consider returning an error or using a more appropriate fallback value.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#discussion_r2413051817
##########
datafusion/functions/src/math/log.rs:
##########
@@ -134,15 +134,60 @@ fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64, ArrowError>
}
}
-/// Binary function to calculate an integer logarithm of Decimal128 `value` using `base` base
-/// Returns error if base is invalid or if value is out of bounds of Decimal128
+/// Binary function to calculate an integer logarithm of Decimal256 `value` using `base` base
+/// Returns error if base is invalid
fn log_decimal256(value: i256, scale: i8, base: f64) -> Result<f64, ArrowError> {
- match value.to_i128() {
- Some(value) => log_decimal128(value, scale, base),
- None => Err(ArrowError::NotYetImplemented(format!(
- "Log of Decimal256 larger than Decimal128 is not yet supported: {value}"
- ))),
+ if !base.is_finite() || base.trunc() != base {
+ return Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer base: {base}"
+ )));
+ }
+ if (base as u32) < 2 {
+ return Err(ArrowError::ComputeError(format!(
+ "Log base must be greater than 1: {base}"
+ )));
+ }
+
+ // Try to convert to i128 for faster calculation if possible
+ if let Some(value_i128) = value.to_i128() {
+ let unscaled_value = decimal128_to_i128(value_i128, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ return Ok(log_value as f64);
+ } else {
+ return Ok(f64::NAN);
+ }
+ }
+
+ // For values that don't fit in i128, use f64 approximation
+ let unscaled_value = decimal256_to_i256(value, scale)?;
+
+ // Check if the value is non-positive
+ if !unscaled_value.is_positive() {
+ return Ok(f64::NAN);
+ }
+
+ // Convert i256 to f64 for logarithm calculation
+ // Note: This may lose precision for very large numbers, but that's acceptable
+ // for logarithm calculation since the result is relatively small
+ let value_f64 = i256_to_f64(unscaled_value);
+ let log_value = value_f64.log(base);
+
+ Ok(log_value.floor())
+}
+
+/// Converts i256 to f64 for logarithm calculation
+/// This may lose precision for very large numbers but is acceptable for log calculation
+fn i256_to_f64(value: i256) -> f64 {
+ // Try to convert directly if it fits in i128
+ if let Some(value_i128) = value.to_i128() {
+ return value_i128 as f64;
Review Comment:
Is it redundant to check this again, as it was checked already in `log_decimal256()`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#issuecomment-3762703228
See
- https://github.com/apache/datafusion/issues/17555#issuecomment-3762702960
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#issuecomment-3629888441
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Implement log for Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey closed pull request #17918: feat: Implement log for Decimal256
URL: https://github.com/apache/datafusion/pull/17918
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org