jayzhan211 commented on code in PR #24934:
URL: https://github.com/apache/datafusion/pull/24934#discussion_r3940052133
##########
datafusion/sqllogictest/src/engines/conversion.rs:
##########
@@ -40,158 +41,83 @@ pub(crate) fn varchar_to_str(value: &str) -> String {
}
}
-pub(crate) fn f16_to_str(value: f16) -> String {
+pub(crate) fn float_to_str<T: Float + ToString>(value: T, round_digits: i64)
-> String {
if value.is_nan() {
// The sign of NaN can be different depending on platform.
// So the string representation of NaN ignores the sign.
"NaN".to_string()
- } else if value == f16::INFINITY {
+ } else if value == T::infinity() {
"Infinity".to_string()
- } else if value == f16::NEG_INFINITY {
+ } else if value == T::neg_infinity() {
"-Infinity".to_string()
} else {
- big_decimal_to_str(BigDecimal::from_str(&value.to_string()).unwrap(),
None)
+ float_decimal_to_str(value, round_digits)
}
}
+pub(crate) fn f16_to_str(value: f16) -> String {
+ float_to_str(value, 12)
+}
+
pub(crate) fn f32_to_str(value: f32) -> String {
- if value.is_nan() {
- // The sign of NaN can be different depending on platform.
- // So the string representation of NaN ignores the sign.
- "NaN".to_string()
- } else if value == f32::INFINITY {
- "Infinity".to_string()
- } else if value == f32::NEG_INFINITY {
- "-Infinity".to_string()
- } else {
- big_decimal_to_str(BigDecimal::from_str(&value.to_string()).unwrap(),
None)
- }
+ float_to_str(value, 12)
Review Comment:
I was wondering why 12 and 15 were chosen here. Here's an explanation from
Claude — maybe we could add a comment capturing this so it's clear to the next
reader.
```rs
/// Decimal places floats are rounded to before comparison.
///
/// Note this is decimal *places*, not significant digits:
`BigDecimal::round`
/// counts from the decimal point, so how much precision survives depends on
the
/// magnitude of the value. Near 1.0 this keeps ~12-13 significant digits and
/// discards the tail where float noise lives — summation order across
/// partitions, FMA, platform libm differences — which is what makes results
/// reproducible across machines and partition counts. Above ~1e5 it is
/// effectively a no-op (the shortest round-trip repr has fewer than 12
/// fractional digits); below ~1e-12 values collapse to `0`.
///
/// The value is empirical, inherited from the original
Postgres-compatibility
/// runner (#4834), not derived from a spec. It is kept because floats are
/// inexact and rounding away their last digits is deliberate: for a test
/// harness, determinism matters more than display fidelity. Decimals are
exact,
/// have no noise to suppress, and are deliberately *not* rounded — see
/// [`arrow_decimal_to_str`].
const FLOAT_ROUND_DIGITS: i64 = 12;
/// Spark's expected values under `test_files/spark` were recorded from real
/// Spark output, which carries more digits than [`FLOAT_ROUND_DIGITS`]
/// preserves. Raised to 15 in #15168 so they stop being truncated; this is
not
/// a Spark rule, and Spark does not specify decimal places.
const SPARK_FLOAT_ROUND_DIGITS: i64 = 15;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
