You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2025/09/16 09:19:51 UTC
[PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
scovich opened a new pull request, #8357:
URL: https://github.com/apache/arrow-rs/pull/8357
# 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.
- Closes #NNN.
# Rationale for this change
Historically, `Variant::as_fXX` methods don't even try to cast int values as floating point, which is counter-intuitive.
# What changes are included in this PR?
Allow lossless casting of variant integer values to variant floating point values, by a naive determination of precision:
* Every floating point number has some number of bits of precision
* 53 (double)
* 24 (single)
* 11 (half)
* Any integer that fits entirely inside the target floating point type's precision can be converted losslessly
* This produces an intuitive result: "too big" numbers fail to convert, while "small enough" numbers do convert.
* This is a sufficient but _not_ a necessary condition.
* Technically, wider integer can be represented losslessly as well, as long as they have enough trailing zeros
* It's unclear whether allowing those wider values to cast is actually helpful in practice, because only 1 in 2**k values can cast (where k is the number of bits of excess precision); it would certainly make input testing more expensive.
# Are these changes tested?
New unit tests and doc tests.
# Are there any user-facing changes?
Yes. Values that failed to cast before now succeed.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#issuecomment-3303523564
🚀
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#discussion_r2354259537
##########
parquet-variant/src/variant.rs:
##########
@@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI)));
///
+ /// // and from integers with no more than 11 bits of precision
Review Comment:
Oh! Good catch on the doc comment. Updated.
##########
parquet-variant/src/variant.rs:
##########
@@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI)));
///
+ /// // and from integers with no more than 11 bits of precision
Review Comment:
Oh! Good catch on the doc comment. Updated all three.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#discussion_r2354157130
##########
parquet-variant/src/utils.rs:
##########
@@ -144,3 +144,20 @@ pub(crate) const fn expect_size_of<T>(expected: usize) {
let _ = [""; 0][size];
}
}
+
+pub(crate) fn fits_precision<const N: u32>(n: impl Into<i64>) -> bool {
Review Comment:
Thanks for the detailed reply, I'm fine with the current implementation
##########
parquet-variant/src/variant.rs:
##########
@@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI)));
///
+ /// // and from integers with no more than 11 bits of precision
Review Comment:
Maybe we need to update the doc(the comment said that -- Returns `Some(f16)` for float and double variants, `None` for non-floating-point variants) or add an example for integer overflow (`Variant::from(2048) for f16 -- will return None)
I don't have a strong preference here, it's just an idea that popped into my mind when I saw this
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#discussion_r2354150806
##########
parquet-variant/src/utils.rs:
##########
@@ -144,3 +144,20 @@ pub(crate) const fn expect_size_of<T>(expected: usize) {
let _ = [""; 0][size];
}
}
+
+pub(crate) fn fits_precision<const N: u32>(n: impl Into<i64>) -> bool {
Review Comment:
It was mostly out of habit for small integer manipulation utilities... But now that you mention, it probably doesn't matter in the slightest -- the compiler will anyway inline aggressively and the constant arg will be folded in regardless of whether it's a generic arg or a function arg.
That said, there's one potential advantage to keeping the generic arg: Otherwise, it could be ambiguous which of two integer args is the precision and which is the actual value. Any preferences?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#issuecomment-3303524459
Thanks @scovich and @klion26
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#discussion_r2354130737
##########
parquet-variant/src/utils.rs:
##########
@@ -144,3 +144,20 @@ pub(crate) const fn expect_size_of<T>(expected: usize) {
let _ = [""; 0][size];
}
}
+
+pub(crate) fn fits_precision<const N: u32>(n: impl Into<i64>) -> bool {
Review Comment:
Put `N` into the generic, not the parameter, because this can yield better performance, do I understand correctly?
##########
parquet-variant/src/variant.rs:
##########
@@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI)));
///
+ /// // and from integers with no more than 11 bits of precision
Review Comment:
Not sure if we need to add an overflow example here(e.g, Variant::from(2048) for f16)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#discussion_r2354152655
##########
parquet-variant/src/variant.rs:
##########
@@ -1096,13 +1096,21 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f16(), Some(f16::from_f64(std::f64::consts::PI)));
///
+ /// // and from integers with no more than 11 bits of precision
Review Comment:
The unit test for `fits_precision` does test overflow, both positive and negative.
Do we need additional (indirect) test coverage here?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Allow lossless casting from integer to floating point [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8357:
URL: https://github.com/apache/arrow-rs/pull/8357#issuecomment-3296972788
CC @klion26 @alamb -- should be a quickie
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org