From e6823e508225c84ff3a5c94041d165ded428d3aa Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 10 Sep 2026 11:52:59 -0600 Subject: [PATCH 1/2] fix: render float and double Iceberg partition values like iceberg-java iceberg-java renders a float or double partition value with `Float.toString` / `Double.toString`, which keeps a fractional digit on a whole value and switches to scientific notation outside [1e-3, 1e7). Comet's partition-path renderer delegated both types to iceberg-rust, whose `Display` does neither, so `Double.MAX_VALUE` became a 309-digit directory name: past the 255-byte limit on a single path component, which failed the write with `File name too long`. Comet already spells Java's rules in the `cast(float as string)` path, so extract that formatting from `cast_float_to_string!` into `write_java_float_string` and call it from both places. The macro becomes a generic function over the arrow float types, and the cast keeps formatting straight into the string builder: the coefficient inspection the scientific branch needs now uses a stack buffer instead of a reused `String`. Closes #5836 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B --- .../operators/iceberg_partition_path.rs | 85 +++++- native/spark-expr/src/conversion_funcs/mod.rs | 1 + .../src/conversion_funcs/numeric.rs | 258 ++++++++++++------ .../comet/CometIcebergWriteActionSuite.scala | 36 +++ 4 files changed, 286 insertions(+), 94 deletions(-) diff --git a/native/core/src/execution/operators/iceberg_partition_path.rs b/native/core/src/execution/operators/iceberg_partition_path.rs index 363a9f2f19e..3213b1e8497 100644 --- a/native/core/src/execution/operators/iceberg_partition_path.rs +++ b/native/core/src/execution/operators/iceberg_partition_path.rs @@ -27,6 +27,7 @@ use std::sync::Arc; use base64::engine::general_purpose::STANDARD as BASE64; use base64::Engine; +use datafusion_comet_spark_expr::{write_java_float_string, JavaFloatString}; use iceberg::spec::{ Literal, PartitionKey, PartitionSpec, PrimitiveLiteral, PrimitiveType, SchemaRef, StructType, Transform, Type, @@ -150,17 +151,18 @@ const NULL: &str = "null"; /// | `timestamp` | `1969-12-31T23:59:58.5` | `1969-12-31 23:59:58.500` | /// | `timestamptz` | `1969-12-31T23:59:58.5+00:00` | panics for a negative value with a sub-second part; otherwise `1969-12-31 23:59:58.500 UTC` | /// | `binary` / `fixed` | base64 | uppercase hex | +/// | `float` / `double` | `1.0`, `1.0E20` | `1`, `100000000000000000000` | /// /// The nanosecond timestamp types get the same treatment for the same reason. They are V3-only, so /// `CometIcebergNativeWrite`'s format-version gate keeps them out of a native write today; the arms /// exist so a future V3 write does not reintroduce the panic. /// -/// Known remaining divergence, deliberately left delegating: `float` and `double`. Java renders -/// them with `Float.toString`/`Double.toString` (always a fractional digit, `E` notation outside -/// `[1e-3, 1e7)`), Rust with its own shortest representation, so `1.0` becomes `1` and `1.0E20` -/// becomes `100000000000000000000`. Porting Java's algorithm is a much larger piece of work than a -/// partition directory name warrants -- Comet's `cast(float as string)` needs the same port -- and -/// unlike `timestamptz` it does not panic. Iceberg deprecated float/double partitioning in 1.3. +/// `float` and `double` go through `write_java_float_string`, the same rendering Comet's +/// `cast(float as string)` uses, because Java spells both with `Float.toString`/`Double.toString`. +/// Rust's `Display` never switches to an exponent, so delegating rendered `Double.MAX_VALUE` as 309 +/// digits, overrunning the 255-byte limit on a single path component and failing the write with +/// `File name too long` (apache/datafusion-comet#5836). Iceberg deprecated float and double +/// partitioning in 1.3, so this is for tables that already have such a field. fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal>) -> String { // Java returns "null" for a null partition value regardless of transform or type, which also // covers every `void` field: `void` produces no value, so this is the only arm it reaches. @@ -188,10 +190,24 @@ fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal Some(PrimitiveType::Binary | PrimitiveType::Fixed(_)), PrimitiveLiteral::Binary(bytes), ) => BASE64.encode(bytes), + (Some(PrimitiveType::Float), PrimitiveLiteral::Float(value)) => java_float_string(**value), + (Some(PrimitiveType::Double), PrimitiveLiteral::Double(value)) => { + java_float_string(**value) + } _ => transform.to_human_string(field_type, value), } } +/// `Float.toString` / `Double.toString` of one partition value, as an owned `String` for +/// `human_string`'s signature. Writing a partition path happens once per data file, so the +/// allocation is not on any hot path. +fn java_float_string(value: T) -> String { + let mut out = String::new(); + // Writing into a `String` cannot fail. + let _ = write_java_float_string(value, &mut out); + out +} + /// Renders a sub-second count since the Unix epoch the way iceberg-java's /// `DateTimeUtil.microsToIsoTimestamp[tz]` / `nanosToIsoTimestamp[tz]` do: /// `DateTimeFormatter.ISO_LOCAL_DATE_TIME` over the UTC `LocalDateTime`, optionally followed by the @@ -388,6 +404,63 @@ mod tests { assert_eq!(civil_from_days(-719_529), (-1, 12, 31)); } + fn double(value: f64) -> String { + human_string( + &Transform::Identity, + &Type::Primitive(PrimitiveType::Double), + Some(&Literal::Primitive(PrimitiveLiteral::Double(value.into()))), + ) + } + + fn float(value: f32) -> String { + human_string( + &Transform::Identity, + &Type::Primitive(PrimitiveType::Float), + Some(&Literal::Primitive(PrimitiveLiteral::Float(value.into()))), + ) + } + + // Expectations produced by `Double.toString` / `Float.toString` on the JDK: plain notation + // inside [1e-3, 1e7) with at least one digit either side of the point, scientific notation + // outside it. Rust's own `Display` never uses an exponent, so `Double.MAX_VALUE` came out as + // 309 digits -- past a filesystem's 255-byte limit on one path component, which failed the + // write outright (apache/datafusion-comet#5836). + #[test] + fn renders_doubles_like_java_double_to_string() { + assert_eq!(double(1.0), "1.0"); + assert_eq!(double(-0.5), "-0.5"); + assert_eq!(double(0.0), "0.0"); + assert_eq!(double(-0.0), "-0.0"); + assert_eq!(double(123.456), "123.456"); + // Boundaries of the plain-notation window, which is closed below and open above. + assert_eq!(double(0.001), "0.001"); + assert_eq!(double(9.99e-4), "9.99E-4"); + assert_eq!(double(9_999_999.0), "9999999.0"); + assert_eq!(double(1.0e7), "1.0E7"); + assert_eq!(double(1.0e20), "1.0E20"); + assert_eq!(double(f64::MAX), "1.7976931348623157E308"); + assert_eq!(double(-f64::MAX), "-1.7976931348623157E308"); + assert_eq!(double(f64::MIN_POSITIVE), "2.2250738585072014E-308"); + assert_eq!(double(f64::from_bits(1)), "4.9E-324"); + assert_eq!(double(f64::NAN), "NaN"); + assert_eq!(double(f64::INFINITY), "Infinity"); + assert_eq!(double(f64::NEG_INFINITY), "-Infinity"); + } + + #[test] + fn renders_floats_like_java_float_to_string() { + assert_eq!(float(1.0), "1.0"); + assert_eq!(float(-0.5), "-0.5"); + assert_eq!(float(0.1), "0.1"); + assert_eq!(float(0.001), "0.001"); + assert_eq!(float(9_999_999.0), "9999999.0"); + assert_eq!(float(1.0e7), "1.0E7"); + assert_eq!(float(f32::MAX), "3.4028235E38"); + assert_eq!(float(f32::from_bits(1)), "1.4E-45"); + assert_eq!(float(f32::NAN), "NaN"); + assert_eq!(float(f32::INFINITY), "Infinity"); + } + // Java base64-encodes binary and fixed partition values; iceberg-rust hex-encodes them. #[test] fn base64_encodes_binary_partition_values() { diff --git a/native/spark-expr/src/conversion_funcs/mod.rs b/native/spark-expr/src/conversion_funcs/mod.rs index a9da6915f97..a8ff0f07a04 100644 --- a/native/spark-expr/src/conversion_funcs/mod.rs +++ b/native/spark-expr/src/conversion_funcs/mod.rs @@ -23,4 +23,5 @@ mod temporal; pub(crate) mod trim; mod utils; +pub use numeric::{write_java_float_string, JavaFloatString}; pub(crate) use string::ymd_to_epoch_day; diff --git a/native/spark-expr/src/conversion_funcs/numeric.rs b/native/spark-expr/src/conversion_funcs/numeric.rs index 61db16d973c..66d309879ac 100644 --- a/native/spark-expr/src/conversion_funcs/numeric.rs +++ b/native/spark-expr/src/conversion_funcs/numeric.rs @@ -29,6 +29,7 @@ use arrow::datatypes::{ Float64Type, Int16Type, Int32Type, Int64Type, Int8Type, }; use num::{cast::AsPrimitive, ToPrimitive, Zero}; +use std::fmt::{self, Write}; use std::sync::Arc; /// Check if DataFusion cast from integer types is Spark compatible @@ -136,95 +137,176 @@ macro_rules! cast_float_to_timestamp_impl { }}; } -macro_rules! cast_float_to_string { - ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{ - - fn cast( - from: &dyn Array, - _eval_mode: EvalMode, - ) -> SparkResult - where - OffsetSize: OffsetSizeTrait, { - use std::fmt::Write; - - let array = from.as_any().downcast_ref::<$output_type>().unwrap(); - - // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the - // result is expressed without scientific notation with at least one digit on either side of - // the decimal point. Otherwise, Spark uses a mantissa followed by E and an - // exponent. The mantissa has an optional leading minus sign followed by one digit to the - // left of the decimal point, and the minimal number of digits greater than zero to the - // right. The exponent has and optional leading minus sign. - // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html - - const LOWER_SCIENTIFIC_BOUND: $type = 0.001; - const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0; - - // Values are formatted straight into the builder, so no intermediate String - // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH - // (16 bytes / value) so typical fractional and scientific outputs like - // "1234.5678" or "-1.4E-45" do not force a mid-loop grow. - let mut builder = GenericStringBuilder::::with_capacity( - array.len(), - array.len() * 16, - ); - // Reused across rows by the scientific-notation path, which has to inspect - // the formatted text before emitting it. - let mut scratch = String::with_capacity(32); - - for value in array.iter() { - let Some(value) = value else { - builder.append_null(); - continue; - }; - let abs = value.abs(); - if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs) - || abs == 0.0 - { - let _ = write!(builder, "{value}"); - if value.fract() == 0.0 { - // Spark always renders a fractional digit; Rust omits it. - let _ = builder.write_str(".0"); - } - builder.append_value(""); - } else if !value.is_finite() { - // NaN and the infinities are excluded by the range check above. - builder.append_value(if value.is_nan() { - "NaN" - } else if value.is_sign_positive() { - "Infinity" - } else { - "-Infinity" - }); - } else if abs.to_bits() == 1 { - // Java's Double.toString / Float.toString are not shortest-roundtrip - // and render the smallest subnormals with more digits than Rust does. - builder.append_value(if value.is_sign_negative() { - concat!("-", $min_value) - } else { - $min_value - }); - } else { - scratch.clear(); - let _ = write!(scratch, "{value:E}"); - match scratch.split_once('E') { - Some((coefficient, exponent)) if !coefficient.contains('.') => { - // Spark keeps the fractional digit Rust drops from a whole - // coefficient. - let _ = builder.write_str(coefficient); - let _ = builder.write_str(".0E"); - builder.append_value(exponent); - } - _ => builder.append_value(&scratch), - } - } - } +/// A float width that Java renders through `Float.toString` / `Double.toString`. +/// +/// The two differ only in the literal text of the smallest subnormal, which Java's algorithm +/// spells with more digits than a shortest-round-trip formatter produces. +pub trait JavaFloatString: Copy + PartialOrd + fmt::Display + fmt::UpperExp { + /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it. + const MIN_SUBNORMAL: &'static str; + /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific. + const PLAIN_LOWER: Self; + const PLAIN_UPPER: Self; + + fn abs(self) -> Self; + fn is_zero(self) -> bool; + fn is_whole(self) -> bool; + fn is_finite(self) -> bool; + fn is_nan(self) -> bool; + fn is_sign_negative(self) -> bool; + /// The value whose magnitude is one ULP above zero, the one Java does not render shortest. + fn is_smallest_subnormal(self) -> bool; +} + +macro_rules! impl_java_float_string { + ($type:ty, $min_subnormal:expr) => { + impl JavaFloatString for $type { + const MIN_SUBNORMAL: &'static str = $min_subnormal; + const PLAIN_LOWER: Self = 0.001; + const PLAIN_UPPER: Self = 10000000.0; - Ok(Arc::new(builder.finish())) + fn abs(self) -> Self { + <$type>::abs(self) + } + fn is_zero(self) -> bool { + self == 0.0 + } + fn is_whole(self) -> bool { + self.fract() == 0.0 + } + fn is_finite(self) -> bool { + <$type>::is_finite(self) } + fn is_nan(self) -> bool { + <$type>::is_nan(self) + } + fn is_sign_negative(self) -> bool { + <$type>::is_sign_negative(self) + } + fn is_smallest_subnormal(self) -> bool { + <$type>::abs(self).to_bits() == 1 + } + } + }; +} - cast::<$offset_type>($from, $eval_mode) - }}; +impl_java_float_string!(f32, "1.4E-45"); +impl_java_float_string!(f64, "4.9E-324"); + +/// Writes `value` as Java's `Float.toString` / `Double.toString` renders it. +/// +/// If the absolute value is less than 10,000,000 and greater or equal than 0.001, the result is +/// expressed without scientific notation with at least one digit on either side of the decimal +/// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having +/// an optional leading minus sign followed by one digit to the left of the decimal point and the +/// minimal number of digits greater than zero to the right. +/// +/// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's +/// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would +/// render as 309 digits. Both matter beyond cosmetics: Spark spells a `cast(double as string)` +/// this way, and iceberg-java spells a float or double partition directory this way, where the +/// unabbreviated form overruns the filesystem's limit on one path component. +/// +/// This is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value. +/// Only the smallest subnormal, by far the most visible case, is corrected for here. +/// +/// Errors only if `out` does; writing into a `String` or an arrow string builder cannot fail. +pub fn write_java_float_string( + value: T, + out: &mut W, +) -> fmt::Result { + let abs = value.abs(); + if (T::PLAIN_LOWER..T::PLAIN_UPPER).contains(&abs) || abs.is_zero() { + write!(out, "{value}")?; + if value.is_whole() { + // Java always renders a fractional digit; Rust omits it. + out.write_str(".0")?; + } + Ok(()) + } else if !value.is_finite() { + // NaN and the infinities are excluded by the range check above. + out.write_str(if value.is_nan() { + "NaN" + } else if value.is_sign_negative() { + "-Infinity" + } else { + "Infinity" + }) + } else if value.is_smallest_subnormal() { + if value.is_sign_negative() { + out.write_str("-")?; + } + out.write_str(T::MIN_SUBNORMAL) + } else { + // The coefficient has to be inspected before any of it is emitted, so it is formatted + // into a stack buffer rather than into `out`, which may not be rewindable. + let mut scratch = ExponentBuf::default(); + write!(scratch, "{value:E}")?; + match scratch.as_str().split_once('E') { + Some((coefficient, exponent)) if !coefficient.contains('.') => { + // Java keeps the fractional digit Rust drops from a whole coefficient. + out.write_str(coefficient)?; + out.write_str(".0E")?; + out.write_str(exponent) + } + _ => out.write_str(scratch.as_str()), + } + } +} + +/// Scratch space for one `{:E}` rendering. The longest a float produces is +/// `-2.2250738585072014E-308`, 24 bytes. +#[derive(Default)] +struct ExponentBuf { + bytes: [u8; 32], + len: usize, +} + +impl ExponentBuf { + fn as_str(&self) -> &str { + // Only `{:E}` output, which is ASCII, is ever written. + std::str::from_utf8(&self.bytes[..self.len]).expect("ascii float text") + } +} + +impl fmt::Write for ExponentBuf { + fn write_str(&mut self, s: &str) -> fmt::Result { + let end = self.len + s.len(); + // Unreachable for `{:E}` of an f32 or f64; returning an error rather than panicking keeps + // a future caller's mistake out of the JNI boundary. + let target = self.bytes.get_mut(self.len..end).ok_or(fmt::Error)?; + target.copy_from_slice(s.as_bytes()); + self.len = end; + Ok(()) + } +} + +/// Casts a float array to strings the way Spark's `cast(float as string)` does, which is Java's +/// `Float.toString` / `Double.toString`. +fn spark_cast_float_to_utf8(from: &dyn Array) -> SparkResult +where + T: ArrowPrimitiveType, + T::Native: JavaFloatString, + OffsetSize: OffsetSizeTrait, +{ + let array = from.as_primitive::(); + // Values are formatted straight into the builder, so no intermediate String is allocated per + // row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH (16 bytes / value) so + // typical fractional and scientific outputs like "1234.5678" or "-1.4E-45" do not force a + // mid-loop grow. + let mut builder = + GenericStringBuilder::::with_capacity(array.len(), array.len() * 16); + for value in array.iter() { + match value { + None => builder.append_null(), + Some(value) => { + // Infallible for a string builder; the signature is generic over the sink. + let _ = write_java_float_string(value, &mut builder); + builder.append_value(""); + } + } + } + Ok(Arc::new(builder.finish())) } // eval mode is not needed since all ints can be implemented in binary format @@ -731,7 +813,7 @@ pub(crate) fn spark_cast_float64_to_utf8( where OffsetSize: OffsetSizeTrait, { - cast_float_to_string!(from, _eval_mode, f64, Float64Array, OffsetSize, "4.9E-324") + spark_cast_float_to_utf8::(from) } pub(crate) fn spark_cast_float32_to_utf8( @@ -741,7 +823,7 @@ pub(crate) fn spark_cast_float32_to_utf8( where OffsetSize: OffsetSizeTrait, { - cast_float_to_string!(from, _eval_mode, f32, Float32Array, OffsetSize, "1.4E-45") + spark_cast_float_to_utf8::(from) } fn cast_int_to_decimal128_internal( diff --git a/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala b/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala index 0c6affcd30a..a6d0ac58183 100644 --- a/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala @@ -1107,6 +1107,42 @@ class CometIcebergWriteActionSuite } } + // iceberg-java renders a `float` or `double` partition value with `Float.toString` / + // `Double.toString`, which keeps a fractional digit on a whole value and switches to scientific + // notation outside [1e-3, 1e7). Rust's `Display` does neither, so iceberg-rust's renderer spelled + // `Double.MAX_VALUE` as 309 digits: past the 255-byte limit on one path component, which failed + // the write with `File name too long` (apache/datafusion-comet#5836). + test("native acceleration: float and double partition paths match iceberg-java") { + assumeNativeAcceleration() + withIcebergCatalog { warehouseDir => + Seq("float_path_native", "float_path_jvm").foreach { table => + spark.sql(s""" + CREATE TABLE $catalog.$ns.$table (id INT, f FLOAT, d DOUBLE) + USING iceberg PARTITIONED BY (f, d) + """) + } + // A whole value, one inside the plain-notation window, and the two extremes that overran + // the path component limit. + val values = + "(1, CAST(1.0 AS FLOAT), CAST(1.0 AS DOUBLE)), " + + "(2, CAST(-0.5 AS FLOAT), CAST(1.7976931348623157E308 AS DOUBLE)), " + + "(3, CAST(3.4028235E38 AS FLOAT), CAST(4.9E-324 AS DOUBLE)), " + + "(4, CAST(0.001 AS FLOAT), CAST(1.0E20 AS DOUBLE))" + + assertNativeWriteEngages("float_path_native", Seq(1, 2, 3, 4)) { + spark.sql(s"INSERT INTO $catalog.$ns.float_path_native VALUES $values") + } + spark.sql(s"INSERT INTO $catalog.$ns.float_path_jvm VALUES $values") + + val nativeDirs = partitionDirs(warehouseDir, "float_path_native") + assert(nativeDirs == partitionDirs(warehouseDir, "float_path_jvm"), s"native: $nativeDirs") + assert(nativeDirs.contains("f=1.0/d=1.0"), s"native: $nativeDirs") + assert(nativeDirs.contains("f=-0.5/d=1.7976931348623157E308"), s"native: $nativeDirs") + assert(nativeDirs.contains("f=3.4028235E38/d=4.9E-324"), s"native: $nativeDirs") + assert(nativeDirs.contains("f=0.001/d=1.0E20"), s"native: $nativeDirs") + } + } + // iceberg-java's `UpdatePartitionSpec` keeps a dropped partition field in a format-version-1 // spec as a `void` transform so its field id survives, and `PartitionSpec#isUnpartitioned` is // "every field is void", not "no fields". The next write therefore runs through the From 649ea49d07eade9f4dcdcdc54be4c573e01e3612 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 10 Sep 2026 12:38:07 -0600 Subject: [PATCH 2/2] refactor: tighten the Java float formatter after review Build `JavaFloatString` on `num::Float`, the bound this crate already uses to abstract f32/f64 next door in `cast_string_to_float_impl`, so the trait keeps only what `num` does not supply: the plain-notation window and the smallest subnormal Java does not render shortest. That drops the second macro and six forwarding methods. Also read the scientific-notation scratch once rather than twice, restore the source citation the format rules lost when they moved out of the macro, note why `identity` is the only transform that reaches the new float and double arms, and assert the partition directories as a set so an unexpected fifth one fails the test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B --- .../operators/iceberg_partition_path.rs | 25 +++---- .../src/conversion_funcs/numeric.rs | 75 +++++++------------ .../comet/CometIcebergWriteActionSuite.scala | 16 ++-- 3 files changed, 47 insertions(+), 69 deletions(-) diff --git a/native/core/src/execution/operators/iceberg_partition_path.rs b/native/core/src/execution/operators/iceberg_partition_path.rs index 3213b1e8497..37c324b4266 100644 --- a/native/core/src/execution/operators/iceberg_partition_path.rs +++ b/native/core/src/execution/operators/iceberg_partition_path.rs @@ -157,12 +157,10 @@ const NULL: &str = "null"; /// `CometIcebergNativeWrite`'s format-version gate keeps them out of a native write today; the arms /// exist so a future V3 write does not reintroduce the panic. /// -/// `float` and `double` go through `write_java_float_string`, the same rendering Comet's -/// `cast(float as string)` uses, because Java spells both with `Float.toString`/`Double.toString`. -/// Rust's `Display` never switches to an exponent, so delegating rendered `Double.MAX_VALUE` as 309 -/// digits, overrunning the 255-byte limit on a single path component and failing the write with -/// `File name too long` (apache/datafusion-comet#5836). Iceberg deprecated float and double -/// partitioning in 1.3, so this is for tables that already have such a field. +/// `float` and `double` go through `write_java_float_string`, which `cast(float as string)` also +/// uses; delegating rendered `Double.MAX_VALUE` as 309 digits and failed the write with `File name +/// too long` (apache/datafusion-comet#5836). Iceberg deprecated float and double partitioning in +/// 1.3, so this is for tables that already have such a field. fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal>) -> String { // Java returns "null" for a null partition value regardless of transform or type, which also // covers every `void` field: `void` produces no value, so this is the only arm it reaches. @@ -172,7 +170,8 @@ fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal // `year`/`month`/`day`/`hour` render the ordinal itself and never see a timestamp or binary // field type (their result types are `int` and `date`), so they cannot collide with the arms - // below. iceberg-rust already mirrors `TransformUtil` for them. + // below. iceberg-rust already mirrors `TransformUtil` for them. `bucket` and `truncate` reject + // float and double outright, so `identity` is the only transform that reaches those two arms. match (field_type.as_primitive_type(), &primitive) { (Some(PrimitiveType::Timestamp), PrimitiveLiteral::Long(micros)) => { iso_timestamp(*micros, 6, false) @@ -198,9 +197,8 @@ fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal } } -/// `Float.toString` / `Double.toString` of one partition value, as an owned `String` for -/// `human_string`'s signature. Writing a partition path happens once per data file, so the -/// allocation is not on any hot path. +/// `Float.toString` / `Double.toString` of one partition value, owned for `human_string`'s +/// signature. A partition path is built once per data file, so the allocation is off the hot path. fn java_float_string(value: T) -> String { let mut out = String::new(); // Writing into a `String` cannot fail. @@ -420,11 +418,8 @@ mod tests { ) } - // Expectations produced by `Double.toString` / `Float.toString` on the JDK: plain notation - // inside [1e-3, 1e7) with at least one digit either side of the point, scientific notation - // outside it. Rust's own `Display` never uses an exponent, so `Double.MAX_VALUE` came out as - // 309 digits -- past a filesystem's 255-byte limit on one path component, which failed the - // write outright (apache/datafusion-comet#5836). + // Expectations taken from `Double.toString` / `Float.toString` output on the JDK + // (apache/datafusion-comet#5836). #[test] fn renders_doubles_like_java_double_to_string() { assert_eq!(double(1.0), "1.0"); diff --git a/native/spark-expr/src/conversion_funcs/numeric.rs b/native/spark-expr/src/conversion_funcs/numeric.rs index 66d309879ac..257efe7fdcf 100644 --- a/native/spark-expr/src/conversion_funcs/numeric.rs +++ b/native/spark-expr/src/conversion_funcs/numeric.rs @@ -28,7 +28,7 @@ use arrow::datatypes::{ i256, is_validate_decimal_precision, ArrowPrimitiveType, DataType, Decimal128Type, Float32Type, Float64Type, Int16Type, Int32Type, Int64Type, Int8Type, }; -use num::{cast::AsPrimitive, ToPrimitive, Zero}; +use num::{cast::AsPrimitive, Float, ToPrimitive, Zero}; use std::fmt::{self, Write}; use std::sync::Arc; @@ -139,59 +139,40 @@ macro_rules! cast_float_to_timestamp_impl { /// A float width that Java renders through `Float.toString` / `Double.toString`. /// -/// The two differ only in the literal text of the smallest subnormal, which Java's algorithm +/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation +/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm /// spells with more digits than a shortest-round-trip formatter produces. -pub trait JavaFloatString: Copy + PartialOrd + fmt::Display + fmt::UpperExp { +pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp { /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it. const MIN_SUBNORMAL: &'static str; /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific. const PLAIN_LOWER: Self; const PLAIN_UPPER: Self; - fn abs(self) -> Self; - fn is_zero(self) -> bool; - fn is_whole(self) -> bool; - fn is_finite(self) -> bool; - fn is_nan(self) -> bool; - fn is_sign_negative(self) -> bool; - /// The value whose magnitude is one ULP above zero, the one Java does not render shortest. + /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value` + /// is the smallest *normal*, so this has no `num` equivalent. fn is_smallest_subnormal(self) -> bool; } -macro_rules! impl_java_float_string { - ($type:ty, $min_subnormal:expr) => { - impl JavaFloatString for $type { - const MIN_SUBNORMAL: &'static str = $min_subnormal; - const PLAIN_LOWER: Self = 0.001; - const PLAIN_UPPER: Self = 10000000.0; +impl JavaFloatString for f32 { + const MIN_SUBNORMAL: &'static str = "1.4E-45"; + const PLAIN_LOWER: Self = 0.001; + const PLAIN_UPPER: Self = 10000000.0; - fn abs(self) -> Self { - <$type>::abs(self) - } - fn is_zero(self) -> bool { - self == 0.0 - } - fn is_whole(self) -> bool { - self.fract() == 0.0 - } - fn is_finite(self) -> bool { - <$type>::is_finite(self) - } - fn is_nan(self) -> bool { - <$type>::is_nan(self) - } - fn is_sign_negative(self) -> bool { - <$type>::is_sign_negative(self) - } - fn is_smallest_subnormal(self) -> bool { - <$type>::abs(self).to_bits() == 1 - } - } - }; + fn is_smallest_subnormal(self) -> bool { + self.abs().to_bits() == 1 + } } -impl_java_float_string!(f32, "1.4E-45"); -impl_java_float_string!(f64, "4.9E-324"); +impl JavaFloatString for f64 { + const MIN_SUBNORMAL: &'static str = "4.9E-324"; + const PLAIN_LOWER: Self = 0.001; + const PLAIN_UPPER: Self = 10000000.0; + + fn is_smallest_subnormal(self) -> bool { + self.abs().to_bits() == 1 + } +} /// Writes `value` as Java's `Float.toString` / `Double.toString` renders it. /// @@ -200,6 +181,7 @@ impl_java_float_string!(f64, "4.9E-324"); /// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having /// an optional leading minus sign followed by one digit to the left of the decimal point and the /// minimal number of digits greater than zero to the right. +/// Source: /// /// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's /// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would @@ -218,7 +200,7 @@ pub fn write_java_float_string( let abs = value.abs(); if (T::PLAIN_LOWER..T::PLAIN_UPPER).contains(&abs) || abs.is_zero() { write!(out, "{value}")?; - if value.is_whole() { + if value.fract().is_zero() { // Java always renders a fractional digit; Rust omits it. out.write_str(".0")?; } @@ -242,20 +224,21 @@ pub fn write_java_float_string( // into a stack buffer rather than into `out`, which may not be rewindable. let mut scratch = ExponentBuf::default(); write!(scratch, "{value:E}")?; - match scratch.as_str().split_once('E') { + let text = scratch.as_str(); + match text.split_once('E') { Some((coefficient, exponent)) if !coefficient.contains('.') => { // Java keeps the fractional digit Rust drops from a whole coefficient. out.write_str(coefficient)?; out.write_str(".0E")?; out.write_str(exponent) } - _ => out.write_str(scratch.as_str()), + _ => out.write_str(text), } } } -/// Scratch space for one `{:E}` rendering. The longest a float produces is -/// `-2.2250738585072014E-308`, 24 bytes. +/// Scratch space for one `{:E}` rendering, sized past the longest a float can produce +/// (`-2.2250738585072014E-308`). #[derive(Default)] struct ExponentBuf { bytes: [u8; 32], diff --git a/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala b/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala index a6d0ac58183..4c4a04feecd 100644 --- a/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala +++ b/spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala @@ -1108,10 +1108,8 @@ class CometIcebergWriteActionSuite } // iceberg-java renders a `float` or `double` partition value with `Float.toString` / - // `Double.toString`, which keeps a fractional digit on a whole value and switches to scientific - // notation outside [1e-3, 1e7). Rust's `Display` does neither, so iceberg-rust's renderer spelled - // `Double.MAX_VALUE` as 309 digits: past the 255-byte limit on one path component, which failed - // the write with `File name too long` (apache/datafusion-comet#5836). + // `Double.toString`. Rust's `Display` spelled `Double.MAX_VALUE` as 309 digits instead, past the + // 255-byte limit on one path component (apache/datafusion-comet#5836). test("native acceleration: float and double partition paths match iceberg-java") { assumeNativeAcceleration() withIcebergCatalog { warehouseDir => @@ -1136,10 +1134,12 @@ class CometIcebergWriteActionSuite val nativeDirs = partitionDirs(warehouseDir, "float_path_native") assert(nativeDirs == partitionDirs(warehouseDir, "float_path_jvm"), s"native: $nativeDirs") - assert(nativeDirs.contains("f=1.0/d=1.0"), s"native: $nativeDirs") - assert(nativeDirs.contains("f=-0.5/d=1.7976931348623157E308"), s"native: $nativeDirs") - assert(nativeDirs.contains("f=3.4028235E38/d=4.9E-324"), s"native: $nativeDirs") - assert(nativeDirs.contains("f=0.001/d=1.0E20"), s"native: $nativeDirs") + assert( + nativeDirs == Set( + "f=1.0/d=1.0", + "f=-0.5/d=1.7976931348623157E308", + "f=3.4028235E38/d=4.9E-324", + "f=0.001/d=1.0E20")) } }